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(MFFileMgmt): fix strip_model_relative_path to avoid IndexError #1748

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Mar 16, 2023

  • Fix Load simulation error "list index out of range" #1747. MFSimulation.load() with absolute or relative paths on Windows could raise IndexError, tracing to MFFileMgmt.strip_model_relative_path. Avoid this and refactor to use pathlib. Test providing namefile by name, by absolute path, and by relative path

  • Since Modflow6 supports POSIX paths even on Windows, use forward slash separators in relative path strings returned by strip_model_relative_path on all OS — previously used backslashes on Windows

  • Convert back- to forward-slashes in filename ctor argument to MFPackage. Fixes an error introduced in fix(MFFileMgmt): remove string_to_file_path #1759 occurring with a relative filename with Windows separators

  • Currently FloPy MFSimulation.write_simulation() and similar still emit backslashes on Windows, in future these could write posix paths too. Add a test case for this but ignore it for now

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #1748 (cd64b6d) into develop (6041835) will increase coverage by 0.1%.
The diff coverage is 100.0%.

❗ Current head cd64b6d differs from pull request most recent head 0d611fc. Consider uploading reports for the commit 0d611fc to get more accurate results

@@            Coverage Diff            @@
##           develop   #1748     +/-   ##
=========================================
+ Coverage     72.0%   72.2%   +0.1%     
=========================================
  Files          254     253      -1     
  Lines        55915   56057    +142     
=========================================
+ Hits         40302   40496    +194     
+ Misses       15613   15561     -52     
Impacted Files Coverage Δ
flopy/mf6/mfbase.py 81.5% <100.0%> (+0.2%) ⬆️
flopy/mf6/mfpackage.py 66.2% <100.0%> (+0.9%) ⬆️

... and 21 files with indirect coverage changes

@langevin-usgs
Copy link
Contributor

Any thoughts @spaulins-usgs?

@wpbonelli wpbonelli marked this pull request as draft April 15, 2023 16:37
@wpbonelli wpbonelli force-pushed the test-paths branch 2 times, most recently from de8efdf to cd64b6d Compare April 15, 2023 22:55
@wpbonelli wpbonelli marked this pull request as ready for review April 15, 2023 23:12
* fix strip_model_relative_path() to avoid indexing error
* test simulation load with filenames, abs paths & rel paths
* add tests for POSIX separator use on all platforms (ignored for now)
* MFPackage fixes: support PathLike, replace Windows pathsep in filename argument
@wpbonelli
Copy link
Member Author

Aside from the MFFileMgmt patch, this PR fixes the regression test failing since #1759 (filename param on MFPackage should support names or rel path, provided as str or PathLike). This could move to its own PR if that would be cleaner.

@langevin-usgs langevin-usgs merged commit 7db9263 into modflowpy:develop Apr 27, 2023
@wpbonelli wpbonelli deleted the test-paths branch April 27, 2023 13:04
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.

Load simulation error "list index out of range"
2 participants