Skip to content
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

fix(resolve_exe): typecast exe name to string before passing to _resolve #2457

Merged
merged 4 commits into from
Feb 20, 2025

Conversation

martclanor
Copy link
Contributor

This PR aims to address #2455. The issue is related to an edge case wherein a missing executable that is being passed to resolve_exe is provided as a pathlib.Path object.

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.8%. Comparing base (bb9824e) to head (3bdfe7f).
Report is 60 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #2457     +/-   ##
=========================================
+ Coverage     68.4%   75.8%   +7.4%     
=========================================
  Files          294     293      -1     
  Lines        59390   61914   +2524     
=========================================
+ Hits         40652   46972   +6320     
+ Misses       18738   14942   -3796     
Files with missing lines Coverage Δ
flopy/mbase.py 72.6% <100.0%> (+2.8%) ⬆️

... and 254 files with indirect coverage changes

@martclanor martclanor marked this pull request as ready for review February 19, 2025 22:04
Copy link
Member

@wpbonelli wpbonelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does moving str() outside the internal _resolve fix it? Figured we were missing a none check somewhere.

I wonder if there is a better way to just to look up a program by name or path. Having introduced this function in the first place I regret its necessity. It is a source of frequent pain

@martclanor
Copy link
Contributor Author

martclanor commented Feb 20, 2025

Inside _resolve, typecasting to string is already done but only for one case (out of 2 cases). I thought instead of adding str() to the other case, it might be better to just ensure that we pass a string object to _resolve. This also eliminates the need to add a test wherein we pass (to resolve_exe) a Path object of an executable that doesn't exist.

Not sure how to change it while keeping it forgiving (e.g. with the add/remove suffix logic), but I'm thinking maybe with pathlib.Path.is_file instead of shutil.which we can make it at least a bit simpler?

@wpbonelli wpbonelli merged commit ca3adb3 into modflowpy:develop Feb 20, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants