test: integrate pylint into bazel - #325
Conversation
furtib
added
enhancement
| --hash=sha256:52f39653876c7dec3e3afd4c2696920e05c83832b9737afc21928f2d2eb7a753 \ | ||
| --hash=sha256:986fed8bcf79fb82c78b18a53352a0b287a73817d6dbcfba3162da36667c49a0 |
There was a problem hiding this comment.
I'm wondering... do we need these?
There was a problem hiding this comment.
The hashes we dont; everything else is necessary. The hashes were just a byproduct of how I generated this file, removed.
| paths = [ | ||
| "src", | ||
| "test", | ||
| "__init__.py", |
There was a problem hiding this comment.
__init__.py we don't I was just messing around and left it in by accident.
| name = "pylint", | ||
| exclude = [ | ||
| "**/__pycache__/**", | ||
| ], |
There was a problem hiding this comment.
Basically by default we should analyze only *.py files everywhere, but maybe exclude some particular files and locations.
There was a problem hiding this comment.
Since there is and always will be just a single entry, I haven't really thought about default values.
| if "external" not in tags: | ||
| pylint_tags.append("external") |
There was a problem hiding this comment.
What is this for may I ask?
There was a problem hiding this comment.
This action does not depend on any of the Python files it analyzes, only the Pylint executable.
Due to this, even if a Python file were to be changed, this action would still be cached.
This tag ensures that this action is never cached.
There was a problem hiding this comment.
OK, so, I guess you refer to Tag conventions
I kind of doubt that we need this tag... Why this action should not be cached?
But if we do there should be (1) evidences and (2) a comment in the code
There was a problem hiding this comment.
It seems you are right, and this rule does recognize if a Python file was changed (I wonder how/why) and invalidates the cache.
That's very interesting.
Scrap that; I found evidence.
Remove the tag from the bzl file and run these commands:
bazel test //test/pylint:pylint --test_output=errors # PASSED (runs)
sed -i '0,/^import argparse$/s//&\nimport calendar' "src/per_file_script.py" # inject W0611
bazel test //test/pylint:pylint --test_output=errors # BUG: (cached) PASSED
bazel test //test/pylint:pylint --nocache_test_results --test_output=errors # FAILED (real)For this to occur, the bazel test must PASS; therefore, I included the patch fixing cache tests.
There was a problem hiding this comment.
It seems you are right, and this rule does recognize if a Python file was changed (I wonder how/why) and invalidates the cache. That's very interesting.
Scrap that; I found evidence.
I think that is the bug of the pylint_test macro - it does not define the dependency on the files it should run pylint on. This better be fixed properly...
Meanwhile, if you "just run pylint" without dependency control, I think you should use no-cache tag instead of external (see Attributes common to all build rules)
2 participants
Why:
We want to run pylint just as easily as Buildifier.
What:
__init__.pyfiles fromsrc/andtest/foss. (Others are necessary for pytest.)Addresses:
none
Depends on: #324.