Replace use of str(pathlib.Path(...)) in codebase #599
Conversation
Signed-off-by: zili <erik.zilinsky@icontest.hu>
Signed-off-by: zili <erik.zilinsky@icontest.hu>
There was a problem hiding this comment.
This looks really good - nice work!
I've flagged a few things for follow up; they almost all fall into one of three camps:
- Calls to stdlib functions where I'm almost certain explicit
fsdecodeconversion isn't required - this is calls likeshutil.rmtree() - Calls to stdlib functions where I suspect explicit
fsdecodeconversion isn't required, but I could be wrong - this is mostly in calls tosubprocesset al, where my gut tells me that Path objects should be OK forcwd, and might be OK for arguments... but I also wouldn't be surprised if they're not. Investigate; if I'm wrong, let me know and move on. - Calls where we are in control of the external API surface. Wherever possible, the API exposed by Briefcase should be the same as stdlib - "string or path like object". We should be managing any
fsdecodeconversion so that the end-user doesn't need to worry about that detail (to the extent possible).
To be clear - what you've done is great, and works fine (as evidenced by the tests passing). The issues I've flagged in group 1 and 2 are subtle edge cases where the conversions you've done aren't wrong - they're just a little over-enthusiastic.
The problems in group 3 are mostly problems you've inherited, and the conversions you've done are doing exactly the right thing, but since we're doubling down on making our usage of Path correct, we should take the extra time and clean up those uses, too.
src/briefcase/platforms/macOS/app.py
Outdated
| self.os.makedirs(str(Path(lib_path).parent)) | ||
| self.shutil.move(str(Path(tmpdir) / 'lib'), lib_path) | ||
| self.os.makedirs(os.fsdecode(Path(lib_path).parent)) | ||
| self.shutil.move(os.fsdecode(Path(tmpdir) / 'lib'), lib_path) |
There was a problem hiding this comment.
Are these 4 conversions necessary? AFAICT, they're all being used in calls to stdlib functions, so they should be auto converted
There was a problem hiding this comment.
shutil.move only accepts path-like objects from 3.9, I think we should use there os.fsdecode() link
There was a problem hiding this comment.
Well that's annoying :-)
In that case, you're definitely correct including the conversion here. However, it would be worth adding a docstring so future editors know (a) this isn't redundant, and (b) when we (eventually) drop 3.8 support, dropping the conversion is an option as well.
| str(support_filename), | ||
| extract_dir=str(support_path) | ||
| os.fsdecode(support_filename), | ||
| extract_dir=os.fsdecode(support_path) |
There was a problem hiding this comment.
Are you sure about this one? On first inspection, it looks like it should be a straight removal of str(), rather than conversion.
There was a problem hiding this comment.
according to docs it should be os.fsdecode or str(): Changed in version 3.7: Accepts a path-like object for filename and extract_dir. link
There was a problem hiding this comment.
As above; definitely correct to include the conversion, but we should document it for future editors.
| @@ -31,7 +32,7 @@ def test_simulator_is_missing(tmp_path): | |||
| simulators = get_simulators( | |||
| command, | |||
| 'iOS', | |||
| simulator_location=str(tmp_path / 'CoreSimulator.framework'), | |||
| simulator_location=os.fsdecode(tmp_path / 'CoreSimulator.framework'), | |||
There was a problem hiding this comment.
Similarly; this method should be modified so it can accept path objects.
There was a problem hiding this comment.
I think we're almost there. All the changes you've made look great (or you've given a good explanation why my comment was wrong).
The handful of places where we've got "Path objects are legal, but not in Python 3.6/3.7" can be flagged with docstrings - something like:
# TODO: Py3.6 compatibility; os.fsencode not required in Py3.7
will give us something to easily search for when the time comes. If you make those handful of changes, I think we'll be good to go!
(As an aside - those docstring updates are sufficiently minor that I would normally make them myself; however, because you've made this PR off your master branch, I can't submit edits. As a general rule, it's a good idea to always do PR work in a branch - firstly, because I allows maintainers to make minor edits like these; but also because it means you can have multiple PRs in flight at the same time. Not a big deal, but if you work on another PR - and I hope you will! - it's worth keeping in mind).
|
@freakboy3742 You can check again the PR, if something is needed please let me know. |
Codecov Report
|
I removed
str()functions fromstr(pathlib.Path(...))or replaced withos.fsdecode.str()for Pathlib objects was introduced for python 3.5 compatibility.Refs #592
PR Checklist: