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: convert lint-python-mutable-default-parameters.sh to Python #24800

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

KevinMusgrave
Copy link
Contributor

This converts one of the linter scripts to Python. Reference issue: #24783

The approach is to just call git grep using subprocess.run.

Alternative approaches could be to use Python instead of git grep (I'm not sure how) or use pylint --disable=all --enable=W0102, though that requires installation of pylint.

@DrahtBot DrahtBot added the Tests label Apr 7, 2022
@laanwj laanwj mentioned this pull request Apr 8, 2022
25 tasks
def main():
command = "git grep -E".split(" ")
command.append(r"^\s*def [a-zA-Z0-9_]+\(.*=\s*(\[|\{)")
command.extend("-- *.py".split(" "))
Copy link
Member

Choose a reason for hiding this comment

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

lets just write this as command.extend(["--", "*.py"])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I put the whole command into an array



def main():
command = "git grep -E".split(" ")
Copy link
Member

@laanwj laanwj Apr 8, 2022

Choose a reason for hiding this comment

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

command = ["git", "grep", "-E"], no need to call split here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I put the whole command into an array

command.append(r"^\s*def [a-zA-Z0-9_]+\(.*=\s*(\[|\{)")
command.extend("-- *.py".split(" "))
output = subprocess.run(command, stdout=subprocess.PIPE, universal_newlines=True)
output = output.stdout
Copy link
Member

@laanwj laanwj Apr 8, 2022

Choose a reason for hiding this comment

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

Would prefer to use output.stdout in following code instead of this assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@maflcko
Copy link
Member

maflcko commented Apr 8, 2022

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Change permission

Change argument so that it's compatiable with python 3.6

Change comment to docstring

Remove .split, .append, .extend calls. Remove 'output' variable assignment
@KevinMusgrave
Copy link
Contributor Author

@MarcoFalke Done

@maflcko
Copy link
Member

maflcko commented Apr 11, 2022

review ACK e8e48fa

@maflcko maflcko merged commit 22e3b6f into bitcoin:master Apr 11, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 11, 2022
…ters.sh to Python

e8e48fa Converted lint-python-mutable-default-parameters.sh to python (TakeshiMusgrave)

Pull request description:

  This converts one of the linter scripts to Python. Reference issue: bitcoin#24783

  The approach is to just call git grep using subprocess.run.

  Alternative approaches could be to use Python instead of git grep (I'm not sure how) or use ```pylint --disable=all --enable=W0102```, though that requires installation of pylint.

ACKs for top commit:
  MarcoFalke:
    review ACK e8e48fa

Tree-SHA512: 7f6f4887dee02c9751b225a6a131fb705868859c4a9af25bb3485cda2358650486b110f17adf89d96a20f212d7d94899922a07aab12c8dc11984cfd5feb7a076
@bitcoin bitcoin locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants