Skip to content

lint: Report all lint errors instead of early exit #28862

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 2 commits into from
Nov 22, 2023
Merged

lint: Report all lint errors instead of early exit #28862

fanquake merged 2 commits into from
Nov 22, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 13, 2023

all-lint.py currently collects all failures. However, the 06_script.sh does not, since July this year (#28103 (comment)).

Fix this by printing all failures before exiting.

Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 13, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Nov 13, 2023
@fanquake fanquake requested review from laanwj and sedited November 17, 2023 11:04
sedited
sedited reviewed Nov 19, 2023
View reviewed changes
sedited
sedited approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

lgtm ACK fa01f88

I checked that the CI can now correctly report multiple lint failures.

@kevkevinpal
Copy link
Contributor

kevkevinpal commented Nov 20, 2023

ACK fa01f88

tested on my own fork and saw multiple lint errors
https://github.com/kevkevinpal/bitcoin/pull/9/checks?check_run_id=18865861937

@maflcko
Copy link
Member Author

maflcko commented Nov 21, 2023

tested on my own fork and saw multiple lint errors

Yeah, I guess linters should enforce their rules globally, instead of only on the diff. Otherwise, one gets different results, depending on how the diff is calculated.

Though, this is unrelated to this pull request and can be improved in the future, if there is need and interest.

@fanquake fanquake merged commit 172cd92 into bitcoin:master Nov 22, 2023
@maflcko maflcko deleted the 2311-lint-all-err- branch November 23, 2023 14:07
@bitcoin bitcoin locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

@laanwj laanwj Awaiting requested review from laanwj

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @kevkevinpal @sedited @fanquake