Use explicit imports - #978
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #375 by making imports in Wflow.jl more explicit (reducing reliance on broad using ... statements and unnecessary module qualification), and cleans up the dependency set by removing an unused Roots.jl entry from the manifest to prevent CI failures.
Changes:
- Replace broad imports / module-qualified references with explicit imports and unqualified in-module references where appropriate.
- Update internal references to
STANDARD_NAME_MAPS,STANDARD_UNITS,pow, andInputSectionto rely on in-module bindings. - Remove unused
Roots.jl(and its now-unneededCommonSolve) fromManifest.toml.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Wflow/src/Wflow.jl | Refines imports and internal references; defines STANDARD_NAME_MAPS; adjusts run! cleanup callsite. |
| Wflow/src/standard_name/standard_name_utils.jl | Uses in-module STANDARD_NAME_MAPS for metadata lookup. |
| Wflow/src/routing/surface/surface_process.jl | Uses in-module pow helper without Wflow. qualification. |
| Wflow/src/io.jl | Uses in-module STANDARD_UNITS in warning message. |
| Wflow/src/config_structure.jl | Uses in-module InputSection for fieldnames constant initialization. |
| Manifest.toml | Removes unused Roots (and CommonSolve) from the manifest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vers-w
left a comment
vers-w
left a comment
There was a problem hiding this comment.
LGTM, just one small comment.
|
|
||
| # option to support running function twice without re-initializing | ||
| # and thus opening the netCDF files | ||
| if close_files |
There was a problem hiding this comment.
Maybe cleaner to rename the keyword argument close_files?
There was a problem hiding this comment.
Agree it would be cleaner, but it would break API for minimal gain, so I don't think it is worth it?
There was a problem hiding this comment.
Ah yes, good point, I agree that renaming is then not worth it.
Issue addressed
Fixes #375
Also removes Roots.jl from the manifest. Not sure why it was there since it wasn't used. I noticed TeamCity failing on this false dependency.
Explanation
I ran ExplicitImports. Not all findings from that package are addressed here. For instance use of non-public names and importing from non-owner modules are not fixed here.