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

Setup for automated code quality #58

Closed
wants to merge 1 commit into from
Closed

Conversation

Batalex
Copy link

@Batalex Batalex commented Apr 3, 2024

As discussed in pyOpenSci/software-submission#157, here is a suggestion on how to automate code quality (but not limited to!) tasks.

The setup is simple, as we use only ruff to both format and lint the files.
I suggest using nox as the task runner.

Running the formatter + linter is just as simple as

nox

for short, or

nox -s format

for the long version.

Tests are also included:

nox -s tests

Now, for the configuration itself, I have added some entries to pyproject.toml with what, I think, is a good first setup for harmonize_wq.

There are things to discuss, so I'll keep this PR as a draft for now.

Comment on lines +5 to +8
RUN_DEPS = ["geopandas", "pint", "dataretrieval"]

FORMAT_DEPS = ["ruff >= 0.3.5"]
TESTS_DEPS = ["pytest", "coverage"]
Copy link
Author

Choose a reason for hiding this comment

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

todo: Ideally, we want to keep those in sync with the deps listed in pyproject.toml.

Comment on lines +41 to +46
select = [
"E",
"F",
"W",
"I"
]
Copy link
Author

Choose a reason for hiding this comment

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

suggestion: You might want to take a look at the list of rules available. This configuration is equivalent to flake8 + isort, but we could want a different set of rules for harmonize_wq

Rules can also be ignored:

  • entirely
  • at the file level
  • for a specifc line using a comment # noqa: E722 for instance

known-first-party = ["harmonize_wq"]

[tool.ruff.format]
quote-style = "single"
Copy link
Author

Choose a reason for hiding this comment

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

discussion: The default format used by black (and ruff as well) is to use double quotes for strings. However, this option will avoid messing with the full code base

@jbousquin
Copy link
Collaborator

Starting to close the loop on this by copying over what I've mentioned elsewhere. The suggested settings have been working well in the pyproject.toml for running ruff locally without nox. I have pre-commits mostly working, but without the settings. I'm trying to work out if there is an easy way to do that from what is in the toml without nox. A lot of pieces I don't have a full grasp on though. What I'm aiming for is to have pre-commit make basic ruff changes on a PR, but to fail for not fixable changes. The idea being a contributor wouldn't have to get ruff/nox running locally to leverage it.

@jbousquin
Copy link
Collaborator

Specific to the settings - I appreciated quote-style = "single", at least for now to give me time to ensure it won't matter. Outside of that the only ones I'm not sure of are: (1) adding space between some import types (I001 - isn't specific to order vs spacing so I may just have to deal with a couple extra empty lines), and (2) E501 Line too long, where I was already linting for this, but not in docs examples where those lines get compared to doctest (as long as I can find a way to fix without failing doctests no issue).

@jbousquin
Copy link
Collaborator

TLDR - implemented w/ PR #89 .

To follow-up on this: part of the discrepancy between local and the pre-commit ended up being that format wasn't being run locally, I used quote-style = "preserve" during initial updates to differentiate where is was just '' vs "". In the end since it was somewhat arbitrary the default "" is used. # noqa: worked on docstring in the end but I tried to resolve using any other means first, over many attempts. All this was merged on PR 89. This PR was very helpful in providing initial settings in the pyproject.toml - thank you! Really the only difference for the merged PR is that it uses pre-commits to run it as part of checks instead of setting up nox to run it local.

@jbousquin
Copy link
Collaborator

whoops - meant to close w/ last comment

@jbousquin jbousquin closed this Aug 3, 2024
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.

2 participants