search

msiemens / tinydb

TinyDB is a lightweight document oriented database optimized for your happiness :)
https://tinydb.readthedocs.org
MIT License
6.84k stars 550 forks source link

Write to a temporary file and rename to avoid damage in case of crash #468

Closed ostafen closed 2 years ago

ostafen commented 2 years ago

PR for #467

ostafen commented 2 years ago

@msiemens Any update on this?

msiemens commented 2 years ago

Hey @ostafen, I've been busy the last couple of weeks, I'll get back to this before the end of this week!

ostafen commented 2 years ago

Ok, thank you for the update 👍 I'm here for any kind of feedback

msiemens commented 2 years ago

After merging this I've added Windows tests for TinyDB and it turns out that os.rename doesn't work on Windows when the destination file already exists (https://github.com/msiemens/tinydb/runs/6561237986?check_suite_focus=true) 🙈 I think I fixed it by adapting some code from https://github.com/untitaker/python-atomicwrites/ but the tests are still running.

It's not your fault that this went unnoticed, I think when you created your PR the CI configuration had an error so that it didn't run on Pull Requests. And even if it did, there were no Windows CI pipelines configured. So this one is on me 🙂

I'll revert the changes for now and try to get this to work on a Windows machine so we can get this feature back in!

msiemens commented 2 years ago

think I fixed it by adapting some code from https://github.com/untitaker/python-atomicwrites/ but the tests are still running.

The tests have run now and they fail even with my workaround: https://github.com/msiemens/tinydb/runs/6561403728. I'll try to take a deeper look soon!