-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
Code Quality Tool Lib #20135
Code Quality Tool Lib #20135
Conversation
c3c4365
to
f84c98a
Compare
@huonw @kaos I think I've added the missing ingredients.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really neat!
I left a few comments. I think we're good to go without them. They're mostly possibilities for improvement.
There's some duplication between the _build_*_rules
. I think that's fine. I think it's better to have 3 easy-to-follow Transaction Scripts than a pile of small "reusable" components (like constructing the FileSpecMatcher).
One thing which we should think about is rule deduplication. Some tools support multiple operations. For example, https://radon.readthedocs.io/en/latest/ has 4 different metrics. If you set up 4 different CodeQualityTools to achieve this, I think each one will generate a separate CodeQualityToolInstance. I'm not sure how much of a problem that will be, since I think that the runnable target will deduplicate the hard work of installing the tool which is where most of the gains would be. Deduplicating isn't that hard, Metalint and other places in Pants do this with a simple cache
decorator.
src/python/pants/backend/experimental/code_quality_tool/lib_test.py
Outdated
Show resolved
Hide resolved
default = () | ||
|
||
|
||
class CodeQualityToolRunnableField(StringField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed, there is some factoring of commonality that needs to be done, which I can take a crack at after these prs are in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks for iterating on this! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, a few things to tweak, but getting very close!
src/python/pants/backend/experimental/code_quality_tool/lib_test.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/experimental/adhoc/code_quality_tool/register.py
Outdated
Show resolved
Hide resolved
621d0a3
to
8bccf82
Compare
@huonw I think the issues you brought up are addressed now. |
Checking in on the status of this. @huonw looks like it needs a last pass over the response to your code review comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Before moving to step 2 of the plan described in #17729 (comment) , cleaning up a gross duplication of rule code that I introduced in #20135 between `adhoc_tool` and the new `code_quality_tool`. This PR extracts the shared logic into the concept of a ToolRunner and a rule to hydrate it in `adhoc_process_support`. Both `adhoc_tool` and `code_quality_tool` have the latent idea of a tool runner and a considerable machinery to build it. Starting from something like ```python @DataClass(frozen=True) class ToolRunnerRequest: runnable_address_str: str args: tuple[str, ...] execution_dependencies: tuple[str, ...] runnable_dependencies: tuple[str, ...] target: Target ``` they need to assemble things like locate the actual runnable by str and figure out what should be its base digest, args, env, etc. and also co-locate the execution and runnable dependencies. We now capture that information as a "runner": ```python @DataClass(frozen=True) class ToolRunner: digest: Digest args: tuple[str, ...] extra_env: Mapping[str, str] append_only_caches: Mapping[str, str] immutable_input_digests: Mapping[str, Digest] ``` After this, `adhoc_tool` and `code_quality_tool` diverge in what they do with it. `adhoc_tool` uses this runner to generate code and code_quality_tool uses it to run batches of lint/fmt/fix on source files. ## Food for thought ... It should not escape our attention that this `ToolRunner` could also be surfaced as a Target, to be used by `adhoc_tool` and `code_quality_tool` rather than each specifying all these fields together. It would also help to reduce confusion when handling all the kinds of 'dependencies' arguments that `adhoc_tool` takes.
This is a library supporting the behavior proposed in #17729 (comment)
The usage example is a bit awkward but there is a lot of variation and discussion about the steps to make the use-case smooth. That work is separated out to a subsequent PR. The core is here.
Goal
Allows a user to define custom linters or formatters by:
code_quality_tool
targetExample Use-case
in
pants.toml
:in
BUILD
:and an in-repo plugin
pants-plugins/flake8_linter_plugin/register.py
The lib supports:
skip_
fields)skip
andonly
working similarly to regular code quality tools.See the tests for demos.
Full demo repo
See this branch:
https://github.com/gauthamnair/example-pants-byo-tool/tree/code-quality-tool-lib-demo
Follow-ups