Fix issue #417: replace os.rename
#418
Conversation
|
CI failed running black for files I didn't modify. This is odd:
Let me know what you want me to do about this (if anything) |
|
Number (1) is because of a different black version, I think. Locally, I was running with black 19.10b0; the latest black is 21.0b0 Number (3) above (which reports in the log as "I/O operation on closed file") is a reported issue on black: psf/black#1664 I'm struggling understand why black changed the formatting for these files at all. In the case of archive.py, it reformatted 2/5 lines that were longer than 79 characters — but the two reformatted lines are 87 characters long (prior to reformatting), which it should have left alone. This feels like a black bug. Perhaps we should be freezing the version of black we use for checking style? E.g., 19.10b0 certainly works (matches the current formatting and doesn't block up with the I/O operation error). |
We're trying to follow the latest versions of all dependencies in the GitHub CI, this allow us to quickly detect potential issues way before we deploy in production. In that case, I have freeze to 19.10b0 to avoid these annoying regressions. We'll try again to remove that constraint once those issues are fixed. |
Instances of `os.rename` in e3 are replaced to use `e3.fs.mv`, which was itself written to replace `shutil.move`. Note that in `e3.fs.mv`, the uses of `os.rename` are retained. Quick testing suggests that these uses do not cause problems, and the error behavior of `os.rename` is sufficiently different from `shutil.move` that I am reluctant to change these uses. In the test case for `e3.fs`, the use of `os.rename` is replaced with `shutil.move` - it seemed self-serving to use `e3.fs.mv` here. Finally, in fix-coverage-paths, the use of `os.rename` is replaced with `shutil.move`. This script doesn't import e3 at all, so sticking with python libraries seemed best.
manthonyaiello
force-pushed
the
feature/fix-#417
branch
from
fde0766
to
fe11aa7
Compare
Codecov Report
@@ Coverage Diff @@
## master #418 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 79 79
Lines 6334 6335 +1
=======================================
+ Hits 6077 6078 +1
Misses 257 257
Continue to review full report at Codecov.
|
Instances of
os.renamein e3 are replaced to usee3.fs.mv, which wasitself written to replace
shutil.move.Note that in
e3.fs.mv, the uses ofos.renameare retained. Quicktesting suggests that these uses do not cause problems, and the error
behavior of
os.renameis sufficiently different fromshutil.movethat I am reluctant to change these uses.
In the test case for
e3.fs, the use ofos.renameis replaced withshutil.move- it seemed self-serving to usee3.fs.mvhere.Finally, in fix-coverage-paths, the use of
os.renameis replaced withshutil.move. This script doesn't import e3 at all, so sticking withpython libraries seemed best.