byuccl / bfasst

Tools for FPGA Assurance Flows
Apache License 2.0
12 stars 5 forks source link

Update make pylint target #402

Closed KeenanRileyFaulkner closed 9 months ago

KeenanRileyFaulkner commented 9 months ago

Currently, the make pylint target checks all .py files in HEAD that differ from the common ancestor of HEAD and origin/main. This seems to be a relic of ensuring that the codebase need not be refactored all at once when pylint was first added to it.

The CI checks handle pylint differently; they simply check all .py files at HEAD in the scripts/ and bfasst/ directories using the following command:

pylint --errors-only $(git ls-files --directory scripts --directory bfasst | grep -e ".py$")

And more broadly uses pylint on all files in the env.GIT_DIFF_FILTERED secret:

pylint ${{ env.GIT_DIFF_FILTERED }}

The make pylint target also does this in addition to the comparison with origin/main. The downside of doing so is that it will error out if files are deleted from the working branch that still exist in main. Because the project has largely been made to follow pylint requirements the check between branches may be unnecessary at this point.

yonnorc42 commented 9 months ago

@KeenanRileyFaulkner How do you feel about this?

pylint: format
    git fetch
    pylint --errors-only $$(git ls-files --directory scripts --directory bfasst | grep -e ".py$$")
    pylint $$(git diff --name-only | grep -e ".py$$")

It gets rid of the error that I was getting yesterday because it's no longer comparing the code to origin/main where the file structure is different from my branch.

KeenanRileyFaulkner commented 9 months ago

Looks fine to me, assuming it doesn't miss anything locally (since it won't impact CI at all).