Skip to content

tests: Port lint-all.sh to lint-all.py #24982

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
merged 1 commit into from
Apr 28, 2022
Merged

tests: Port lint-all.sh to lint-all.py #24982

merged 1 commit into from
Apr 28, 2022

Conversation

hiagopdutra
Copy link
Contributor

@hiagopdutra hiagopdutra commented Apr 25, 2022 •
edited
Loading

This PR is converting test/lint/lint-all.sh to test/lint/lint-assertions.py. It's an item of #24783.

@hiagopdutra hiagopdutra force-pushed the port_lint-all.sh_to_python branch from b84ccb1 to 8cd3644 Compare April 25, 2022 19:57
@laanwj laanwj mentioned this pull request Apr 25, 2022
Closed
25 tasks
@hiagopdutra
Converting lint-all.sh to lint-all.py.
29f44fe 
Converting `lint-all.sh` to `lint-all.py`.
@hiagopdutra hiagopdutra force-pushed the port_lint-all.sh_to_python branch from 8cd3644 to 29f44fe Compare April 26, 2022 09:25
@jacobpfickes
Copy link
Contributor

Ack 29f44fe

This is my first review but the changes seem straightforward enough. Pulled the PR and ensured it ran all lint files as intended. Looks good!

maflcko
maflcko reviewed Apr 27, 2022
View reviewed changes
test/lint/lint-all.py

exit_code = 0
mod_path = Path(__file__).parent
for lint in glob(f"{mod_path}/lint-*"):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for lint in glob(f"{mod_path}/lint-*"):
for lint in glob(f"{mod_path}/lint-*.py"):

wondering if we can make this py

Copy link
Member

Choose a reason for hiding this comment

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

If yes, we can run the subtests with sys.executable instead of calling out to python3

Copy link
Member

Choose a reason for hiding this comment

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

It can be *.py after #24840. I'd also like a lint to prevent someone adding a non-Python linter again 😄 Though it's possibly out of scope of this PR.

@laanwj
Copy link
Member

laanwj commented Apr 28, 2022

Tested ACK 29f44fe
It works just like the bash script. Any improvements can be done later. While running this I noticed I had never actually run "lint-files.py" locally, which turned up a problem with that script, but that's not the fault of this change.

@laanwj laanwj changed the title Converting lint-all.sh to lint-all.py. tests: Port lint-all.sh to lint-all.py Apr 28, 2022
@laanwj laanwj merged commit 85aea18 into bitcoin:master Apr 28, 2022
@hiagopdutra hiagopdutra deleted the port_lint-all.sh_to_python branch April 28, 2022 11:04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 29, 2022
@laanwj
Merge bitcoin#24982: tests: Port lint-all.sh to lint-all.py
19c5708
@maflcko maflcko mentioned this pull request Jun 7, 2022
Merged
@maflcko maflcko mentioned this pull request Jan 3, 2023
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

+2 more reviewers

@maflcko maflcko maflcko left review comments

@jacobpfickes jacobpfickes jacobpfickes approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Docs Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hiagopdutra @jacobpfickes @laanwj @maflcko @DrahtBot