feat(pathlike): support pathlike throughout public interface#1730
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1730 +/- ##
=========================================
+ Coverage 71.9% 72.1% +0.1%
=========================================
Files 253 253
Lines 56004 56043 +39
=========================================
+ Hits 40282 40416 +134
+ Misses 15722 15627 -95
|
4118618 to
b9023dc
Compare
mwtoews
left a comment
There was a problem hiding this comment.
Thanks, looks good so far, see following comments and suggestions. Are there a few representative tests that use str remaining?
38f4499 to
10cc5a0
Compare
|
Added/expanded a few tests covering both |
f09aba2 to
1b4102d
Compare
c5e98f3 to
d0b83c6
Compare
mwtoews
left a comment
There was a problem hiding this comment.
Nice stuff, getting closer, see attached suggestions/comments. I'm primarily looking at the main module, and have not seen many of the edits to notebooks or autotests.
christianlangevin
left a comment
There was a problem hiding this comment.
Looks great @w-bonelli. Nice addition. As you mentioned, a few remaining os.path.join remain in notebooks, but we can get those later.
| output_filename = str( | ||
| Path(output_filename).expanduser().absolute() |
There was a problem hiding this comment.
Out of curiosity, why is str needed here?
There was a problem hiding this comment.
it's not strictly — previously output_filename was a str, switching to Path would require changes to L160 and L523 and I figured it may be safer to keep it a string in case any external usages had similar expectations
97cdea4 to
2b241c7
Compare
|
@mwtoews thanks for the catches, I think they've all been fixed. |
3db3072 to
008f1c5
Compare
mwtoews
left a comment
There was a problem hiding this comment.
Just a few (probably) final comments. And again, I'm only reviewing the changes in flopy/*
| model_ws="./", | ||
| scrub=False, | ||
| exe_name: Union[str, os.PathLike], | ||
| namefile: Optional[str], |
There was a problem hiding this comment.
Currently this is a required parameter, because it does not have a default e.g. = None.
There was a problem hiding this comment.
The docstring indicated namefile=None could be used for models that don't require namefiles — the function seems to handle this case OK, just omits it from the args passed to the executable
- support pathlike or str paths in public APIs - add verbose flag to more utility functions - add sim_path property to MFSimulation - update some notebooks to use pathlib - update most autotests to use pathlib - add unit tests for path arguments - update docstrings and type hints
dfc6829 to
99903b6
Compare
99903b6 to
628c937
Compare
|
Fixed more docstrings/typehints, thanks @mwtoews, also updated exe resolve function introduced in #1727
|
mwtoews
left a comment
There was a problem hiding this comment.
Thanks, looks good! I only have a minor suggestion remaining. This PR could be merged soon.
Working toward #1729, continuation of #1712, #1603, and #1583
os.Pathlikein user-facing APIPathinstead ofstrPathandstrfor core classes/utilitiesverboseflag to more utility methods to optionally enable/suppress outputpathlib, but not allsim_pathproperty toMFSimulationas a shortcut tosimulation_data.mfpath.get_sim_path()Pathwhether user provides workspace asstrorPathLikestrproperties where this was already the conventionBaseModel.model_wsPathin future, but may break usagesresolve_exefunction, add tests, and use it from bothBaseModelas well asrun_model()