Use Pathlib #149
Use Pathlib #149
Conversation
setup.py
Outdated
| @@ -56,10 +56,6 @@ def _hash(filepath): | |||
| out_name = url.split("/")[-1] | |||
| dst_path = external_dir_single / out_name | |||
| if dst_path.is_file(): | |||
| # TODO: boyu: Please review removal of this | |||
There was a problem hiding this comment.
@wang-boyu: We're suggesting removing this "if/else" because the code ends up doing the same thing no matter how the evaluation turns out. A
| # Used for downloading e.g. Leaflet single file | ||
| if out_name is None: | ||
| out_name = url.split("/")[-1] | ||
| dst_path = os.path.join(external_dir_single, out_name) | ||
| if os.path.isfile(dst_path): | ||
| if integrity_hash and (_hash(dst_path) == integrity_hash): |
There was a problem hiding this comment.
@wang-boyu : This if/else block doesn't appear to have any function, so we're suggesting removing it.
There was a problem hiding this comment.
Thanks for pointing this out! This looks like a bug introduced via #61. It was meant to compare the hash with existing file if there's any, and avoids downloading if the hashes match.
Instead of removing it, I would suggest to fix the issue. Perhaps we could delete the return after else statement, and indent some of the code below into else?
setup.py
Outdated
|
|
||
| if dst_path.is_file(): | ||
| return | ||
| if integrity_hash and (_hash(str(dst_path)) != integrity_hash): |
There was a problem hiding this comment.
@wang-boyu Thank you! We think this is the logic you're looking for (note that we changed == to !=); the setup should stop if the hash is provided and does not match.
|
I tried to install directly from the pip install git+https://github.com/catherinedevlin/mesa-geo.git@use-pathlibThen there would be the following error: Building wheels for collected packages: Mesa-Geo
Building wheel for Mesa-Geo (pyproject.toml) ... error
error: subprocess-exited-with-error
× Building wheel for Mesa-Geo (pyproject.toml) did not run successfully.
│ exit code: 1
╰─> [5 lines of output]
running bdist_wheel
running build
running build_py
Downloading the leaflet.js dependency from the internet...
error: [Errno 2] No such file or directory: 'mesa_geo/visualization/templates/css/external'
[end of output]
note: This error originates from a subprocess, and is likely not a problem with pip.
ERROR: Failed building wheel for Mesa-Geo
Failed to build Mesa-Geo
ERROR: Could not build wheels for Mesa-Geo, which is required to install pyproject.toml-based projectsSo it appears that some external dependencies directories were not successfully created. But if I check out this PR and run |
|
Builds seem to be failing in same way. |
|
@wang-boyu Thank you for the catch - it doesn't happen with plain "pip install -e ." But it seems like creating the parents fixes it. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
=======================================
Coverage 80.36% 80.36%
=======================================
Files 10 10
Lines 657 657
Branches 136 136
=======================================
Hits 528 528
Misses 114 114
Partials 15 15
☔ View full report in Codecov by Sentry. |
|
Thanks for the fix! It is now working as expected. Two last things from my side:
@rht is there any other changes you would like to make? |
|
Fixed isort error and squashed commits. Merging now. |
|
I missed the emails, but yeah LGTM. |
Pathlib is a new(-ish) standard library module (introduced 3.4) that provides a cleaner and more reliable way to handle file paths than the old os.path methods. They bake in OS-safety, adapting to Windows back-slashes and Unix forward-slashes seamlessly. Switching to Pathlib cleans up the code a little and avoids possible Windows problems.
We also corrected some logic in the checking the hash of downloaded files in setup.py.