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 imports and ruff for peilbeheerst #120

Open
visr opened this issue Jul 30, 2024 · 0 comments
Open

Fix imports and ruff for peilbeheerst #120

visr opened this issue Jul 30, 2024 · 0 comments

Comments

@visr
Copy link
Member

visr commented Jul 30, 2024

Ruff is an essential tool to automatically help us maintain this codebase and avoid errors. With the large PRs merged this week we also added a lot of exceptions to be able to merge it. I think we should stop doing that as much as possible. I have already enabled a lot more checks and thereby found and fixed a lot of issues. The remaining issues are mostly related to from x import * being used in the peilbeheerst code. I tried to tackle that as well but stranded halfway, leaving main in a ruff-broken state (it was broken before but we ignored it). So now I challenge @rbruijnshkv and co. to pick up the remaining issues. I care about import * especially because using it prevents a lot of other checks from working well. Also it makes the code harder to read, and hence is widely discouraged.

You can run this to see the currently 53 errors and start ticking them off:

pixi run ruff check --fix

An example:

src\peilbeheerst_model\peilbeheerst_model\preprocess_data\HHSK.ipynb:cell 30:1:39: F821 Undefined name `peilgebied`
  |
1 | HHSK["peilgebied"] = gpd.GeoDataFrame(peilgebied[["code", "nen3610id", "globalid", "geometry"]])

If that is fixed, please continue by removing these two lines and fixing the new errors that appear:

Ribasim-NL/ruff.toml

Lines 13 to 14 in 6ea29f2

"F403", # TODO https://docs.astral.sh/ruff/rules/undefined-local-with-import-star/
"F405", # TODO https://docs.astral.sh/ruff/rules/undefined-local-with-import-star-usage/

Happy to assist where I can :)

visr added a commit that referenced this issue Aug 13, 2024
Fixes a part of #120. The goal for this PR is to resolve all 53 open
ruff errors on main, without adding exceptions. Currently the commits in
this PR bring it down to 29. The remaining errors are all of the form
`F821 Undefined name`, where the name is:

- dm_netwerk_path
- gdf_rhws
- model_name
- output_folder
- ribasim_toml
- stored_trc

For various files. Will look with @rbruijnshkv to resolve the rest.

---------

Co-authored-by: rbruijnshkv <[email protected]>
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

No branches or pull requests

1 participant