search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
898 stars 198 forks source link

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

Closed rockett-m closed 2 months ago

rockett-m commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

Got the checks passing. I have Python 3.10-3.12 enabled as those are compatible with pylint. Python 3.13 doesn't support it yet.

rockett-m commented 3 months ago

I think we need virtualenv at this point to work with installing on mac/darwin. When a user already has one of the supported python versions installed (3.10-3.12) this case arises. If pip is run on a specific python version to install python packages for that version...it throws an error due to homebrew and pip clashing. Two options to resolve this are to 1) use virtual environments for install 2) use this flag with pip '--break-system-packages' which I know we will not go with Using pip without a version specifier is not guaranteed to install the packages for the version we the script attempts on. It would only occur if the python default version happens to be the version we try the installs on. And we don't want to be constrained to defaults or modify user defaults. @HalosGhost @madars if you want to have input on this

rockett-m commented 3 months ago

deadsnakes ppa repository (python for ubuntu) is currently undergoing an update and unavailable. Temporary CI/CD failures from this fyi

rockett-m commented 3 months ago

@HalosGhost should be good to merge now if you can take a look, thanks

rockett-m commented 3 months ago

Update - squashed the changes into a single commit. Removed any README changes from this PR

HalosGhost commented 3 months ago

The README updates to run the python linting and the associated environment should be included in this PR; the formatting changes to the README should not.

rockett-m commented 3 months ago

A couple more questions, mostly w.r.t. the .pylintrc file (some also from the previous review iteration)

@rockett-m can you perhaps link the reference you are using to craft the .pylintrc file? Would be good for me to take a look I think

@maurermi This is the standard way to generate the pylintrc file and how I did it before customizing it $ pylint --generate-rcfile > .pylintrc

maurermi commented 3 months ago

A couple more questions, mostly w.r.t. the .pylintrc file (some also from the previous review iteration) @rockett-m can you perhaps link the reference you are using to craft the .pylintrc file? Would be good for me to take a look I think

@maurermi This is the standard way to generate the pylintrc file and how I did it before customizing it $ pylint --generate-rcfile > .pylintrc

Great thanks for linking this, it is surprising to me how verbose the default rcfile is -- but having seen that now I am more comfortable with the verbosity of the one included here

rockett-m commented 3 months ago

If #273 gets in first (change shell script indentation to 4 spaces) then I'll update indents on these shell scripts to 4 spaces right before merge - when all non-style issues have been fixed

HalosGhost commented 3 months ago

Please make sure you go through all of the comments/open-discussions. There are still several of Michael's that don't appear to have been addressed yet.