Chore: More typing for utils/Files and utils/DockerUtils#13677
Chore: More typing for utils/Files and utils/DockerUtils#13677
Conversation
|
|
||
| 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
Test Results - Preflight, Unit23 088 tests 21 229 ✅ 6m 23s ⏱️ Results for commit 45a1baef. |
Test Results (amd64) - Acceptance7 tests 5 ✅ 3m 0s ⏱️ Results for commit 45a1baef. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 34m 27s ⏱️ Results for commit 45a1baef. |
LocalStack Community integration with Pro 2 files 2 suites 1h 57m 7s ⏱️ Results for commit 45a1baef. |
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