-
Notifications
You must be signed in to change notification settings - Fork 66
No atomicwrites #343
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
No atomicwrites #343
Conversation
lucc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far.
Can you please add a docstring and type hints like in the rest of the code.
If you have the time you could also migrate the pytest tests from the original atomicwrites repo to stdlib unittest framework. But I recognize that this is a bit more work. (Test would be nice though so if you do not have the time I will look into it)
And I added a commit which I would ask you to squash into the "revert" commit because I want to keep some of the nix code changes.
This partially reverts commit 10977b7. Some other `nix` changes are preserved so that this just removes the `atomicwrites` bits.
cae032b to
e4156bf
Compare
|
I added some docstrings. I don't really know what typehints do with |
240c739 to
9e6842a
Compare
Lifted from pimutils/khal#1393 (which I also authored). Note that `khal` used `mode='wb'` everywhere while `khard` uses the `atomicwrites` default mode of `w`. Fixes: lucc#342
Seems it was due to something with atomicwrites?
9e6842a to
b5889e4
Compare
|
I didn't drag the tests over; I don't really have a modern Python dev set up to iterate on that rapidly. |
|
If I read the code correctly, there's only one single call to the |
|
Not as a |
|
To be more precise, open-coding it would push the actual "meat" of the logic in by 2 more indentations and decorate it with a dozen or so lines of "safety handling" noise (at least as far as reviewing the call site logic is concerned). |
|
I see, thanks for the explanation! |
|
I migrated the original tests from atomicwrites. Some are still broken, maybe we can fix them or we might disable them. @untitaker is it ok for you if I copy your MIT licensed tests into this GPL project (thus publishing a GPL version of the changed code)? See b6cce2a. |
AFAIK you can include MIT licensed code in GPL licensed project without the need to re-license (and therefore with out approval of / consulting with the author of) that piece of code. Of course you'd have to leave the original copyright note in place. |
This edge case was found by the migrated unit tests from the original atomicwrites repo.
|
Thanks @mathstuf ! I removed the tests that I migrated. I will open a followup PR for that and wait for untitaker there. |
Fixes: #342.