-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Lint on GitHub Actions via pre-commit #104
Conversation
.github/workflows/lint.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I prefer to use pre-commit.ci
, but I know that this is easier to setup in that it doesn't require giving the pre-commit.ci
bot permissions on this repo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why-not-both.gif
? :)
People who fork this repo might enable the CI for testing for their fork, and find lint issues sooner. (And not every lint issue is auto-fixable when a PR is opened.)
Fail fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why-not-both.gif
? :)
no reason except CO2 emissions :)
@@ -50,5 +55,24 @@ local_scheme = "no-local-version" | |||
|
|||
[tool.black] | |||
|
|||
[tool.ruff] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of autofixes, so we could add fix = true
for local use. But we can leave adding that to a followup PR; we'd want to check exactly which rules we're okay with being autofixed etc.
I never got hooked to pre-commits, they felt too slow for me. But with only ruff and black and the few fast things that will get skipped most of the times, it should be OK, I'm willing to try :) LGTM. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -50,5 +55,25 @@ local_scheme = "no-local-version" | |||
|
|||
[tool.black] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly we could enable this setting for black. I don't much like the magic trailing comma, and I think most of the current black maintainers don't really like it much either. But I also don't care that much, if you disagree :)
[tool.black] | |
[tool.black] | |
skip-magic-trailing-comma = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind too much. This is the only change it would make:
diff --git a/sphinxlint/cli.py b/sphinxlint/cli.py
index c0a5638..0cab6d4 100644
--- a/sphinxlint/cli.py
+++ b/sphinxlint/cli.py
@@ -76,11 +76,7 @@ def parse_args(argv=None):
help="verbose (print all checked file names)",
)
parser.add_argument(
- "-i",
- "--ignore",
- action="append",
- help="ignore subdir or file path",
- default=[],
+ "-i", "--ignore", action="append", help="ignore subdir or file path", default=[]
)
parser.add_argument(
"-d",
I can see there could be value keeping it, if we wanted to make sure each parser.add_argument
block had each arg on its own line. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally vote for my suggestion above^, but no strong opinion either way. Feel free to merge!
Yeah, pre-commit --the thing that runs before you commit -- definitely isn't for everyone, and I don't want to make anyone install it locally if they don't want to. A good thing about
|
Yeah, I hate running checks before committing, but love the tool |
I was using tox for all linters :) Feel free to merge. |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [docs/sphinx-lint](https://togithub.com/sphinx-contrib/sphinx-lint) ([changelog](https://togithub.com/sphinx-contrib/sphinx-lint/releases)) | `==0.8.2` -> `==0.9.1` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/docs%2fsphinx-lint/0.9.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/docs%2fsphinx-lint/0.9.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/docs%2fsphinx-lint/0.8.2/0.9.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/docs%2fsphinx-lint/0.8.2/0.9.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>sphinx-contrib/sphinx-lint (docs/sphinx-lint)</summary> ### [`v0.9.1`](https://togithub.com/sphinx-contrib/sphinx-lint/releases/tag/v0.9.1) [Compare Source](https://togithub.com/sphinx-contrib/sphinx-lint/compare/v0.9.0...v0.9.1) #### What's Changed - Add `tool.hatch.build.targets.wheel` to fix `pip install .` with Hatchling 1.19 by [@​hugovk](https://togithub.com/hugovk) in [https://github.com/sphinx-contrib/sphinx-lint/pull/106](https://togithub.com/sphinx-contrib/sphinx-lint/pull/106) This fixes `ValueError: Unable to determine which files to ship inside the wheel using the following heuristics: [...]` when trying to `pip install .`, including via pre-commit. - Add tox for easy testing of multiple Python versions by [@​hugovk](https://togithub.com/hugovk) in [https://github.com/sphinx-contrib/sphinx-lint/pull/100](https://togithub.com/sphinx-contrib/sphinx-lint/pull/100) - Lint on GitHub Actions via pre-commit by [@​hugovk](https://togithub.com/hugovk) in [https://github.com/sphinx-contrib/sphinx-lint/pull/104](https://togithub.com/sphinx-contrib/sphinx-lint/pull/104) **Full Changelog**: sphinx-contrib/sphinx-lint@v0.9.0...v0.9.1 ### [`v0.9.0`](https://togithub.com/sphinx-contrib/sphinx-lint/releases/tag/v0.9.0) [Compare Source](https://togithub.com/sphinx-contrib/sphinx-lint/compare/v0.8.2...v0.9.0) #### What's Changed - Print error messages to stderr by [@​rffontenelle](https://togithub.com/rffontenelle) in [https://github.com/sphinx-contrib/sphinx-lint/pull/102](https://togithub.com/sphinx-contrib/sphinx-lint/pull/102) #### New Contributors - [@​rffontenelle](https://togithub.com/rffontenelle) made their first contribution in [https://github.com/sphinx-contrib/sphinx-lint/pull/102](https://togithub.com/sphinx-contrib/sphinx-lint/pull/102) **Full Changelog**: sphinx-contrib/sphinx-lint@v0.8.2...v0.9.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "every weekend" in timezone Etc/UTC, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/canonical/craftcraft). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuOTMuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Follow on from #100.