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

Move packages within src and fix extra things #295

Merged
merged 24 commits into from
Sep 25, 2024
Merged

Move packages within src and fix extra things #295

merged 24 commits into from
Sep 25, 2024

Conversation

dalonsoa
Copy link
Collaborator

Description

Moves swmmanywhere and netcomp to a src folder. And then updates pyproject.toml and a variety of other paths here and there to work with this new layout.

Tests pass, documentation builds and the generated wheel contains the right things (both swmmanywhere and netcomp can be imported after installing the wheel on a fresh environment), but I have not check trying to run a full script so I guess there might be some hidden things that need fixing.

Fixes #290 again

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

@barneydobson
Copy link
Collaborator

@dalonsoa I don't understand these ruff errors - if I correct them to what is in the failed workflow action - it moves them back when I run pre-commit on my machine...

@cheginit
Copy link
Collaborator

For coverage you need:

[tool.coverage.report]
exclude_lines = [
  "if TYPE_CHECKING:",
]
omit = [
  "**/__init__.py",
]
ignore_errors = true

[tool.coverage.paths]
source = [ "src", "*/site-packages" ]
omit = [
  "**/__init__.py",
]

[tool.coverage.run]
branch = true
source = [
  "swmmanywhere",
]
omit = [
  "**/__init__.py",
]

Also, instead of:

only-include = ["src"]
sources = ["src"]

you can just do:

packages = [ "src" ]

@dalonsoa
Copy link
Collaborator Author

@dalonsoa I don't understand these ruff errors - if I correct them to what is in the failed workflow action - it moves them back when I run pre-commit on my machine...

Not sure. I've just re-run pre-commit locally and all works now.

@github-actions github-actions bot enabled auto-merge September 25, 2024 14:09
@barneydobson
Copy link
Collaborator

barneydobson commented Sep 25, 2024

I followed advice in : astral-sh/ruff#5667 - not sure if that's what did it but the pre-commit seemed to correct itself now..

not sure this is satisfying as pre-commit will now fail locally...

@dalonsoa
Copy link
Collaborator Author

For coverage you need:

[tool.coverage.report]
exclude_lines = [
  "if TYPE_CHECKING:",
]
omit = [
  "**/__init__.py",
]
ignore_errors = true

[tool.coverage.paths]
source = [ "src", "*/site-packages" ]
omit = [
  "**/__init__.py",
]

[tool.coverage.run]
branch = true
source = [
  "swmmanywhere",
]
omit = [
  "**/__init__.py",
]

Could you elaborate why we need all of these?

@dalonsoa
Copy link
Collaborator Author

I followed advice in : astral-sh/ruff#5667 - not sure if that's what did it but the pre-commit seemed to correct itself now

I enabled precommit.ci, which heals some errors automatically, creating new commits with the fixes. See #296

@barneydobson
Copy link
Collaborator

I followed advice in : astral-sh/ruff#5667 - not sure if that's what did it but the pre-commit seemed to correct itself now

I enabled precommit.ci, which heals some errors automatically, creating new commits with the fixes. See #296

Not sure that fixes this though - since it is failing locally but passing after pre commit heal?

@dalonsoa
Copy link
Collaborator Author

dalonsoa commented Sep 25, 2024

Runs locally for me locally on the latest version of this branch with no problems.

@dalonsoa
Copy link
Collaborator Author

Everything nice and green :)

image

@cheginit
Copy link
Collaborator

Could you elaborate why we need all of these?

Coverage doesn't work nice with src-layout by default, so some of these options are required to make it correctly detect the lines and report. Also, I usually omit the init to clean up the report.

@dalonsoa
Copy link
Collaborator Author

Done!

@barneydobson
Copy link
Collaborator

I dunno - pre-commit run --all-files is still doing ruff corrections on this branch on my machine - I will try on a different machine, maybe I have some weirdness going on on here

@cheginit
Copy link
Collaborator

OK, there's an important missing block:

license-files = { paths = ["LICENSE", "src/netcomp/LICENSE.txt"] }

@@ -12,13 +12,13 @@
import cytoolz.curried as tlz
import geopandas as gpd
import joblib
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be down with the swmmanywhere imports no? ruff is moving this in pre-commit on my machine back down to swmmanywhere but precommit.ci is moving it back on here... @cheginit do you have any ideas here as @dalonsoa is unavailable at the mo

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be for the import netcomp line

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this about isort? If so, you need to specify what are the first-party packages.

lint.isort.known-first-party = [
  "swmmanywhere",
  "netcomp",
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

It shouldn't include src, just the name of the package. The pre-commit error sounds like an issue with the service.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK - shall I just abandon precommit.ci and return it to what we had previously for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh lol it has passed now - maybe was a one off

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it sometimes happens, so don't sweat it 😄

@barneydobson barneydobson self-requested a review September 25, 2024 15:17
@github-actions github-actions bot merged commit 2b4eeda into main Sep 25, 2024
5 checks passed
@barneydobson barneydobson deleted the use_src branch September 25, 2024 15:24
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.

netcomp not working properly
3 participants