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

Add python linting workflow to check quality of python code #265

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

rockett-m
Copy link
Contributor

@rockett-m rockett-m commented Jun 10, 2024

See issue #264

This runs on pull-requests and push operations to the codebase. It rates all the python code out of 10 and all style/syntax errors are found in the report. Having a score < 10 will not fail the run as I specified.

Users can manually run pylint --rcfile=.pylintrc $(git ls-files '*.py') to see what lint errors are found in their python code before uploading.

I see the code at 5.68/10.0 now so I set the minimum score as 5.0/10.0 to get it working:
[07/15 update]: python code is now at 9.10/10.0. Using --fail-under=8.0 for the pylint in the ci flow
pylint --rcfile=.pylintrc $(git ls-files '*.py') --fail-under=5.0

[done] In the future I would like to fix the majority of the python lint warnings to get above 8.0/10.0 or 9.0/10.0. We could raise the fail threshold to one of those values once the python files are that quality to make sure future commits meet that standard as well.

Validated simulating this task in the workflow (sub job under the CI task) with act using this command:
act -j pylint --container-architecture linux/amd64
And the workflow passed.

@HalosGhost HalosGhost linked an issue Jun 11, 2024 that may be closed by this pull request
1 task
@rockett-m rockett-m force-pushed the cicd/python-linting branch 15 times, most recently from e8ff602 to 7323f53 Compare June 13, 2024 19:49
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

Thank you for all the cleanup work; this looks like an excellent code quality assurance addition to the CI!

@HalosGhost
Copy link
Collaborator

HalosGhost commented Jun 13, 2024

@rockett-m note that CI is currently failing. Once it's passing, I think I'm ready to merge!

Also, I'm not sure we want to lint the python code against all python versions greater than 3.10 (if my comment about targeting python 3.10+ suggested that, my apologies for the confusion).

I only meant that we probably only needed to require that people have 3.10+ installed (rather than requiring 3.10 exactly).

@rockett-m rockett-m force-pushed the cicd/python-linting branch 10 times, most recently from da6d51e to 5e8f9e9 Compare June 15, 2024 05:16
Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

A couple of comments from me, I think this is improved quite a bit but just would like some clarity on some points

.pylintrc Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/plot.py Outdated Show resolved Hide resolved
scripts/plot.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rockett-m rockett-m left a comment

Choose a reason for hiding this comment

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

Addressed the suggestions here - will upload code with the fixes shortly. Only part still in the air is if we want to limit the method names to declare instance attributes. I vote in favor as it is best practice and we shouldn't need more than these few listed below:

# List of method names used to declare (i.e. assign) instance attributes.
defining-attr-methods=__init__,
                      __new__,
                      setUp,
                      asyncSetUp,
                      __post_init__

@rockett-m rockett-m force-pushed the cicd/python-linting branch 6 times, most recently from 0b571b6 to 3c94678 Compare July 2, 2024 19:23
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/install-build-tools.sh Outdated Show resolved Hide resolved
scripts/activate-venv.sh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rockett-m rockett-m left a comment

Choose a reason for hiding this comment

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

Added in fixes

Will need to make a new issue/PR to improve retrieving the root dir per conversation with @HalosGhost. Can make many scripts more robust from that especially if not cloned via git

scripts/activate-venv.sh Outdated Show resolved Hide resolved
tools/bench/parsec/evm/contracts/gen_header.py Outdated Show resolved Hide resolved
scripts/activate-venv.sh Outdated Show resolved Hide resolved
@rockett-m rockett-m force-pushed the cicd/python-linting branch 2 times, most recently from b2a46ef to a75b1d9 Compare July 10, 2024 08:17
@HalosGhost HalosGhost self-requested a review July 15, 2024 20:15
Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

2 nits, one small-but-necessary change, and the rebase to handle the README updates from #277.

Note: as this PR now includes modifications to native-system-benchmark.sh, I need to do at least one deeper test to make sure local-testing remains fully-operational (note-to-self: shouldn't be a big deal since it should only affect the tx-sample processing at end-of-test).

scripts/activate-venv.sh Outdated Show resolved Hide resolved
scripts/activate-venv.sh Outdated Show resolved Hide resolved
scripts/pylint.sh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rockett-m rockett-m left a comment

Choose a reason for hiding this comment

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

Updated the code to spec and did a rebase to merge in the README.md changes

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

T-ACK. This looks good to me. Adds support for linting python code both to the repo/scripts and leverages it in the CI/CD, and expends some effort into generally improving the quality of python code we have now.

I'm happy to merge if @maurermi agrees.

@rockett-m rockett-m requested a review from maurermi July 17, 2024 18:08
Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

There is one breaking change in contracts/package.json, otherwise this looks good to me.

If others have also tested this code and seen correct results (besides the change in package.json) I am comfortable with this being merged in.

tools/bench/parsec/evm/contracts/package.json Outdated Show resolved Hide resolved
This commit made with the assistance of github copilot

Signed-off-by: Morgan Rockett <[email protected]>
…t-dci#264

This commit made with the assistance of github copilot

Signed-off-by: Morgan Rockett <[email protected]>
Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

T-ACK. This looks good to me, revises some existing code for clarity and then manages defining an environment for python code in this repo. Further will help to clean up future python additions.

@HalosGhost HalosGhost merged commit 86a55a6 into mit-dci:trunk Jul 17, 2024
7 checks passed
@HalosGhost HalosGhost mentioned this pull request Jul 17, 2024
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.

Add linting workflow for python code to keep code quality high
3 participants