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

Initial setup of QA pre-commit tools [cf-units] #522

Merged
merged 30 commits into from
Nov 21, 2024

Conversation

ukmo-ccbunney
Copy link
Contributor

@ukmo-ccbunney ukmo-ccbunney commented Nov 15, 2024

🚀 Pull Request

Description

Added following pre-commit hooks:

  • Extra "default" hooks from pre-commit (trailing whitespace, etc)
  • MyPy
  • codespell
  • validate_pyproject
  • numpydoc

sp-repo-review was already setup for cf-units, but have updated the ignore list as various tests now pass.

ruff was enabled, but with only selected tests. Have enabled "all" tests.

For all QA tools above, the results of any "auto-fixes" have been included in this repo, mainly fixes for:

  • trailing whitespace
  • missing newline at end of file
  • docstring reformatting
  • some code style linting from ruff

All other failing tests have been added to ignore lists. See separate issues:

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 81.48148% with 10 lines in your changes missing coverage. Please review.

Project coverage is 58.40%. Comparing base (4ad25ba) to head (2f88721).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cf_units/__init__.py 80.76% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
- Coverage   58.41%   58.40%   -0.02%     
==========================================
  Files          62       62              
  Lines        6435     6433       -2     
  Branches     1150     1150              
==========================================
- Hits         3759     3757       -2     
  Misses       2385     2385              
  Partials      291      291              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

ukmo-ccbunney and others added 4 commits November 15, 2024 15:54
- Add trailing / to mypy exclude paths
Added extra ignores in mypy for _udunits generated code
Re-added PY005 test to sp-repo-review ignore list.
@ukmo-ccbunney
Copy link
Contributor Author

@pp-mo I Have resolved the issues I was having with pre-commit.ci.
I managed to reproduce the pre-commit.ci behaviour on my personal laptop which gave me a chance to investigate the issue.

I have reinstated the PY005 check ("has test folder") to the to [tool.repo-review] ignore list.

The MyPy behaviour was more difficult to debug. It turns out that adding files to the MyPy exclude list only stops them being passed as filenames to the MyPy call. However, these will still be checked if they are imported from some other module that is being checked.

There is a flag that can be passed to stop this --follow-imports=silent, but this is not supported via pyproject.toml.
However, I've found it can be set using the following too MyPy section:

[[tool.mypy.overrides]]
module = [
        "cf_units/_udunits2_parser/parser.*",
        "cf_units/_udunits2_parser/_antlr4_runtime.*",
]
ignore_errors = true

The combination of the above fix the errors on my personal laptop and the pre-commit.ci service, so this is ready for review now.

I don't know why the behaviour is different on the MO VDIs though (pre-commit and hook version numbers are same!)

@pp-mo
Copy link
Member

pp-mo commented Nov 18, 2024

--follow-imports=silent, but this is not supported via pyproject.toml

I don't think turning this totally off is a good idea, anyway.
Your approach to exclude certain files seems better to me.

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Ok but a few nit-picky suggestions.

You should also create some PRs to track unfinished business.
Suggest one each for :

  • re-order pre-commit entries based on updated tempate file : (can do this one right now)
  • mypy
  • ruff
  • numpydocs-validation

cf_units/_udunits2_parser/_antlr4_runtime/Recognizer.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@ukmo-ccbunney
Copy link
Contributor Author

ukmo-ccbunney commented Nov 19, 2024

You should also create some PRs to track unfinished business. Suggest one each for :

I've already created Issues for these (not PRs), see links in description

Done; see d831bfb

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

I think removing "D212" from the ruff.lint select is worth doing,
not least because the comment is confusing + I think wrong!

otherwise all looks great now + good to go !

pyproject.toml Show resolved Hide resolved
This also updated one docstring in cf_units/__init__.py (correctly) due
to the removal of a redundant `noqa: E501` marker in a previous commit
@ukmo-ccbunney
Copy link
Contributor Author

Right - D212 is back in the Ruff select list after ALL.
Hopefully this is everything that is needed now for this PR.

@pp-mo pp-mo merged commit 03aed8e into SciTools:main Nov 21, 2024
16 checks passed
@ukmo-ccbunney ukmo-ccbunney deleted the qa_reresh_pre-commit-hooks branch November 21, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants