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

[Merged by Bors] - fix(lint-style): make sure Python style errors fail the linting step; fix --fix mode #15954

Closed
wants to merge 6 commits into from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Aug 19, 2024

We need to pass any --fix flag down to lint-style.py; this detail got lost along the rewrite.

While at it, tweak the formatting of error messages slightly: avoid an extra newline after the Python output,
and pluralise errors more correctly.


Open in Gitpod

@grunweg grunweg added the t-linter Linter label Aug 19, 2024
Copy link

github-actions bot commented Aug 19, 2024

PR summary 4ae9f71f08

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

No declarations were harmed in the making of this PR! 🐙

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

@grunweg grunweg changed the title fix(lint-style): make sure Python style errors fail the build fix(lint-style): make sure Python style errors fail the linting step Aug 19, 2024
@grunweg grunweg added the easy < 20s of review time. See the lifecycle page for guidelines. label Aug 19, 2024
While at it, tweak the formatting of error messages slightly:
avoid an extra newline after the Python output,
and pluralise errors more correctly.

Now, we ought to use allUnexpectedError.size for checking:
otherwise, we may print about how to fix 0 errors :-)
@grunweg grunweg force-pushed the MR-lint-style-error-reporting branch from bf17116 to 24514a4 Compare August 19, 2024 00:42
@bryangingechen
Copy link
Contributor

Thanks for looking into this! It looks like reviewdog didn't trigger on 24514a4 (log). Compare this log where reviewdog did run.

@grunweg
Copy link
Collaborator Author

grunweg commented Aug 19, 2024

Good catch - there was another hidden bug (the --fix option was not percolating all the way down to lint-style.py).
I tested it locally, not it worked. Does it also work for you?

@grunweg grunweg changed the title fix(lint-style): make sure Python style errors fail the linting step fix(lint-style): make sure Python style errors fail the linting step; fix --fix mode Aug 19, 2024
@bryangingechen
Copy link
Contributor

Yep, I merged this branch into #15934 and reviewdog is picking up the errors now! Great work!

bors r+

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Aug 19, 2024
@bryangingechen bryangingechen added the CI Modifies the continuous integration / deployment setup label Aug 19, 2024
mathlib-bors bot pushed a commit that referenced this pull request Aug 19, 2024
… fix --fix mode (#15954)

We need to pass any --fix flag down to `lint-style.py`; this detail got lost along the rewrite.

While at it, tweak the formatting of error messages slightly: avoid an extra newline after the Python output,
and pluralise errors more correctly.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Aug 19, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title fix(lint-style): make sure Python style errors fail the linting step; fix --fix mode [Merged by Bors] - fix(lint-style): make sure Python style errors fail the linting step; fix --fix mode Aug 19, 2024
@mathlib-bors mathlib-bors bot closed this Aug 19, 2024
@mathlib-bors mathlib-bors bot deleted the MR-lint-style-error-reporting branch August 19, 2024 02:31
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
… fix --fix mode (#15954)

We need to pass any --fix flag down to `lint-style.py`; this detail got lost along the rewrite.

While at it, tweak the formatting of error messages slightly: avoid an extra newline after the Python output,
and pluralise errors more correctly.
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
… fix --fix mode (#15954)

We need to pass any --fix flag down to `lint-style.py`; this detail got lost along the rewrite.

While at it, tweak the formatting of error messages slightly: avoid an extra newline after the Python output,
and pluralise errors more correctly.
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 12, 2024
… fix --fix mode (#15954)

We need to pass any --fix flag down to `lint-style.py`; this detail got lost along the rewrite.

While at it, tweak the formatting of error messages slightly: avoid an extra newline after the Python output,
and pluralise errors more correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Modifies the continuous integration / deployment setup easy < 20s of review time. See the lifecycle page for guidelines. ready-to-merge This PR has been sent to bors. t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants