-
-
Notifications
You must be signed in to change notification settings - Fork 357
Code clean up. #2097
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
Code clean up. #2097
Conversation
…cs references to PEP for dep groups and ruff changes for upgrade.
|
Need to remove the 3.9 envs, will create a commit to handle that. |
ofek
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 to me so far! I'm not sure what's up with the CI error.
| shutil.rmtree(self, ignore_errors=False) | ||
|
|
||
| def move(self, target: Path) -> None: | ||
| def move(self, target): |
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.
Can we move this method to the end of the class underneath a version check for < 3.14? https://docs.python.org/3/library/pathlib.html#pathlib.Path.move
If you still are encountering static analysis errors at that point then let's just ignore them with comments.
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.
I was torn on this, but if we agree to ignore the static analysis with comments that is fine with me.
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.
Well it works now.
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.
Unresolving this temporarily: could you please check if the behavior of the standard library function is the same (I think it is) and if so move the definition here under an if statement so that it isn't defined on 3.14+?
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.
It is not the same logic. In 3.14, the logic is actually
def move(self, target):
"""
Recursively move this file or directory tree to the given destination.
"""
# Use os.replace() if the target is os.PathLike and on the same FS.
try:
target = self.with_segments(target)
except TypeError:
pass
else:
ensure_different_files(self, target)
try:
os.replace(self, target)
except OSError as err:
if err.errno != EXDEV:
raise
else:
return target.joinpath() # Empty join to ensure fresh metadata.
# Fall back to copy+delete.
target = self.copy(target, follow_symlinks=False, preserve_metadata=True)
self._delete()
return target
And it definitely handles things differently.
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.
I'm okay with switching to the new version and updating logic/tests accordingly. My preference is to always rely on built-in functionality. If this is a bit much of a lift feel free to simply rename this method to something else, that should be quick.
|
This coverage error is probably going to take some time to figure out why it blows up only on test_auth of all things. |
| shutil.rmtree(self, ignore_errors=False) | ||
|
|
||
| def move(self, target: Path) -> None: | ||
| def move(self, target): |
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.
Unresolving this temporarily: could you please check if the behavior of the standard library function is the same (I think it is) and if so move the definition here under an if statement so that it isn't defined on 3.14+?
Co-authored-by: Ofek Lev <ofekmeister@gmail.com>
ofek
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.
Nice, thanks!
| shutil.rmtree(self, ignore_errors=False) | ||
|
|
||
| def move(self, target: Path) -> None: | ||
| def move(self, target): |
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.
I'm okay with switching to the new version and updating logic/tests accordingly. My preference is to always rely on built-in functionality. If this is a bit much of a lift feel free to simply rename this method to something else, that should be quick.
* Code clean up. Remove 3.9, decouple hatch from hatchling, clean up docs references to PEP for dep groups and ruff changes for upgrade. * Remove GHA workflows for 3.9. Fix 1 ruff error * Change test env list to remove old python versions * Fix types and overloads to handle 3.14 * Reformat * Fix typing issues for 3.14 * Remove unused import * Add 3.14 Classifier for Metadata * Implement feedback for docs, fix bad typing change, less abstract name for function * Fix imports from name change * Fix import for normalize fn name change * Revert change to spec and fs * Fix failing tests * Adjust todo message * Format TODO * Add version check for pyfakefs fix * Fix missing import * Remove pyfakefs, used for 1 test and causes issues with 3.14 * Remove other pyfakefs refs * Remove skips to test * Handle test failure for 3.14 version constraint * Update distributions * Handle both standalone sources * Apply suggestion from @ofek Co-authored-by: Ofek Lev <ofekmeister@gmail.com> * Address PR comments --------- Co-authored-by: Ofek Lev <ofekmeister@gmail.com> ff4b404
Some chore clean up tasks.
Closes: #2096