Repository navigation
Chore: More typing for utils/Files and utils/DockerUtils - #13677
Conversation
bblommers
added
semver: patch
|
|
||
| if os.path.exists(file_or_str): | ||
| file_or_str = load_file(file_or_str) | ||
| file_or_str = load_file(file_or_str) # type: ignore[assignment] |
There was a problem hiding this comment.
load_file can return None if the file does not exist. Either the load_file method should be more stringent in what it returns, or we should explicitly verify here that load_file returns something useful.
Either way - I didn't want to change the logic in any way, that's why I just added the type: ignore
| if is_debian(): | ||
| try: | ||
| return run(f'rm -rf "{path}"') | ||
| return run(f'rm -rf "{path}"') # type: ignore[return-value] |
There was a problem hiding this comment.
None of the callers of this method seem to actually use the return-type, so I think it would be safe to change this to:
run(f'rm -rf "{path}"')
returnBut again - I didn't want to change the logic and break things, hence the type: ignore
| if "dirs_exist_ok" in inspect.getfullargspec(shutil.copytree).args: | ||
| kwargs["dirs_exist_ok"] = True |
There was a problem hiding this comment.
The dirs_exist_ok argument was added in Python 3.8, so I think we're OK to always pass the argument.
Docs: https://docs.python.org/3/library/shutil.html#shutil.copytree
bblommers
changed the title
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 41m 9s ⏱️ Results for commit 85cfbe3. ♻️ This comment has been updated with latest results. |
|
@dfangl @dominikschubert Seems like this PR slipped through and is just waiting for a review? |
alexrashed
left a comment
alexrashed
left a comment
There was a problem hiding this comment.
Jumping in here instead of @dfangl and @dominikschubert to get this in.
The changes are looking good, thanks for the additional context on the changes which are not only typing changes (but they are all looking good)! The more typing the better! 💯
45a1bae to
85cfbe3
Compare
Motivation
This PR adds/corrects some more type hints, primarily to the
utils/files.pyand any related utilities. Themypyconfiguration was also updated to now verify that these hints are correct.Changes
Most of the changes are straightforward - I'll add some inline comments if it may not be immediately obvious.
Tests
There should not be any logical changes - but I did run the upstream tests against this branch just in case, and everything passed.
Related
#13658