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

Lint on GitHub Actions via pre-commit #104

Merged
merged 7 commits into from
Nov 24, 2023
Merged

Conversation

hugovk
Copy link
Collaborator

@hugovk hugovk commented Nov 24, 2023

Follow on from #100.

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Collaborator

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 :)

Copy link
Collaborator Author

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.

Copy link
Collaborator

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]
Copy link
Collaborator

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.

@JulienPalard
Copy link
Collaborator

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>
Copy link
Collaborator

@AlexWaygood AlexWaygood left a 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]
Copy link
Collaborator

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 :)

Suggested change
[tool.black]
[tool.black]
skip-magic-trailing-comma = true

Copy link
Collaborator Author

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. 🤷

Copy link
Collaborator

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!

@hugovk
Copy link
Collaborator Author

hugovk commented Nov 24, 2023

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.

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 pre-commit -- the tool -- is that you don't have to enable it to run when you git commit locally, and it's still very useful for:

  • defining all these extra hooks/plugins/tools
  • making it easy to run as Git pre-commit locally
  • making it easy to run as a standalone lint tool locally
  • making it easy to run on the CI, with the same pinned tool versions
  • making it easy to update the pins

@AlexWaygood
Copy link
Collaborator

Yeah, I hate running checks before committing, but love the tool pre-commit, for exactly the reasons @hugovk lists above

@JulienPalard
Copy link
Collaborator

JulienPalard commented Nov 24, 2023

I was using tox for all linters :)

Feel free to merge.

@hugovk hugovk merged commit 74db402 into sphinx-contrib:main Nov 24, 2023
21 checks passed
@hugovk hugovk deleted the add-lint branch November 24, 2023 23:26
lengau pushed a commit to canonical/craftcraft that referenced this pull request Dec 15, 2023
[![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 [@&#8203;hugovk](https://togithub.com/hugovk) in
[sphinx-contrib/sphinx-lint#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
[@&#8203;hugovk](https://togithub.com/hugovk) in
[sphinx-contrib/sphinx-lint#100
- Lint on GitHub Actions via pre-commit by
[@&#8203;hugovk](https://togithub.com/hugovk) in
[sphinx-contrib/sphinx-lint#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
[@&#8203;rffontenelle](https://togithub.com/rffontenelle) in
[sphinx-contrib/sphinx-lint#102

#### New Contributors

- [@&#8203;rffontenelle](https://togithub.com/rffontenelle) made their
first contribution in
[sphinx-contrib/sphinx-lint#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>
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.

4 participants