Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 23, 2022

Conversation

ostafen
Copy link
Contributor

@ostafen ostafen commented Feb 21, 2022

PR for #467

@ostafen
Copy link
Contributor Author

ostafen commented Mar 16, 2022

@msiemens Any update on this?

@msiemens
Copy link
Owner

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

@ostafen
Copy link
Contributor Author

ostafen commented Mar 21, 2022

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

@msiemens msiemens merged commit 2a2abe5 into msiemens:master May 23, 2022
@msiemens
Copy link
Owner

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
Copy link
Owner

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!

richardsheridan pushed a commit to richardsheridan/tinydb that referenced this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants