Deprecate FmtTaskMixin's --skip option #8346
Comments
We need more than per-language, though. We need per-tool |
|
I'm curious whether we will actually need Some amount of research into "why people use |
I would think if they don't want to use the tool. Take Pants: we are not yet ready to use Black and would want to skip it. The tangible motivation here is that I wanted to port isort to V2, but then that would mean when running Likewise, users may not want to run tools like Scalafmt, Pylint, or MyPy. |
|
Mm, true. Using skip to "disable a task in a repo" is another usecase, but there are other models: 1) incremental rollout per target, 2) disabling a task by not installing the plugin that it is in. |
True. Although, the second doesn't cover all situations because some formatters/linters are provided in core Pants, like isort and black. The first one could work, although is not very ergonomic to have to configure every target as not using the tool. I do think it's a good user experience to simply be able to set four lines in [fmt.isort]
skip: True
[lint.isort]
skip: True |
|
Wait, the above got me thinking...We can simply set the It would be nice to simply say: [isort]
skip: TrueOr [isort]
enabled: TrueRather than: [isort.fmt]
skip: True
[isort.lint]
skip: TrueI think this is a better user experience and it solves the problem of how to get per-tool flags to each rule as we already consume these Subystems in the rules. |
|
Yeah, that's not bad. In that case, I prefer |
So, arguably we should be doing less of that, and more contrib moduling (or "backend"ing, at least), so that people can completely disable a plugin like this. It would be useful to think through the whole thing, I think. If we have an enable/skip flag, what would the default be if a particular linter/formatter was alone in a backend? And if it were grouped in with a bunch of other linters? |
Even so, there are cases where a contrib module has multiple logically-grouped functions, with the linter/formatter(s) only being one of them. For example, Python has multiple linters. Let's say we move all of these out of Instead, we could just release
I don't think I follow - what do you mean? I was thinking that the default is always disabled. If we wanted, we could make the decision for each tool based on the convention of the community. For example, clippy and rustfmt seem to be used nearly unanimously in the Rust community, so those could be enabled by default. Meanwhile, Pylint, Black, and isort have very high usage but many projects do not use them so we would default to disabled. |
That feels like a good argument for putting them each in their own plugins, perhaps. Unless we think that the overhead is too high to create a plugin? Unknown. |
|
Opened #8672 to show what this proposal would look like for V2 Black. |
This works for me. I can definitely also see that micro plugins would be a good answer to this; it's slightly higher overhead for one-off set-up (hopefully only negligibly), but feels like potentially a better way to be explicit (and also, avoids polluting the rule graph with unnecessary rules). I guess the core union, result, etc types would probably still be in Pants proper, and plugins would just add implementations that use them. I don't have a strong feeling between the two - either work for me :) |
|
I think multiple backends in a single plugin might work. So we publish a single I want to avoid making publishing more onerous than it already is, we already maintain ~20 pypi projects. But this seems like a good compromise. |
|
Also, @Eric-Arellano FYI, "contrib" is a historical name we should probably move away from. There's no significant distinction between "core pants" and "contrib" in practice. So new rules can all go under |
…_plugins` (#8683) ### Problem We need some mechanism to turn off autoformatters and linters in V2 other than the current `--fmt-{isort,scalafmt,etc}-skip`, which doesn't work because that uses a task-specific option system. Ideally, the option will be tool-specific, so you can say something like: ```ini [black] enable: True [mypy] enable: False ``` However, the solution should also be hygienic. If, for example, someone doesn't want to use Black, then we don't want Black's rules to pollute the rule graph. Nor do we want to make rule authors provide a no-op implementation. ### Solution We decided in #8346 to turn each linter into its own plugin, meaning it may be enabled via the `backend_packages` option like this: ```ini [GLOBAL] backend_packages: +[ "pants.backend.python.lint.black", "pants.backend.python.lint.isort", "pants.backend.scala.lint.scalafmt", ] ``` When specified, that plugin will be enabled, meaning that its options may be configured and its rules will be automatically registered against `fmt` and `lint`. ### Result We have a scalable and explicit mechanism for users to enable formatters and linters in a way that also makes it effortless for rule authors to have a way to opt-out of their tool. This only implements Black. In a followup, we will implement isort under the plugin `pants.backend.python.lint.isort`. For now, V1 tasks will keep the same mechanism as before. We could move them towards this new plugin system, but save that for a followup if we choose to make the deprecations necessary to move V1 users towards the new system.
|
Relates to #8817. |
Closes #8346. This is necessary to rename `fmt2` to `fmt` and `lint2` to `lint`. For the deprecation message, we cannot customize the message based on which task is run. Ideally, given `--fmt-isort-skip`, we could say `Use --isort-skip instead`, but this does not work as the option only gets registered at the root `lint` and `fmt` goals and then gets applied recursively to tasks. Instead, we print this: ``` ▶ ./pants fmt.isort --skip Scrubbed PYTHONPATH=/Users/eric/DocsLocal/code/projects/pants/src/python: from the environment. 21:55:47 [WARN] /Users/eric/DocsLocal/code/projects/pants/src/python/pants/bin/pants_loader.py:85: DeprecationWarning: DEPRECATED: option 'skip' in scope 'fmt.isort' will be removed in version 1.27.0.dev0. `--fmt-skip` and `--lint-skip` are being replaced by options on the linters and formatters themselves. For example, `--fmt-isort-skip` is now `--isort-skip`. Please use these new options instead: * --fmt-go-skip -> --gofmt-skip * --fmt-google-java-format-skip -> --google-java-format-skip * --fmt-isort-skip -> --isort-skip * --fmt-javascriptstyle-skip -> --eslint-skip * --fmt-scalafix-skip -> --scalafix-skip * --fmt-scalafmt-skip -> --scalafmt-skip * --lint-checkstyle-skip -> --checkstyle-skip * --lint-go-skip -> --gofmt-skip * --lint-google-java-format-skip -> --google-java-format-skip * --lint-javascriptstyle-skip -> --eslint-skip * --lint-mypy-skip -> --mypy-skip * --lint-python-eval-skip -> --python-eval-skip * --lint-scalafix-skip -> --scalafix-skip * --lint-scalafmt-skip -> --scalafmt-skip * --lint-scalastyle-skip -> --scalastyle-skip * --lint-thrift-skip -> --scrooge-linter-skip There is no alternative for the top-level `--fmt-skip` and `--lint-skip`. Instead, don't run `./pants fmt` and `./pants lint`. ```
Goal-level options mixins don't work well with v2, because we don't have Tasks to pass the options down to.
We could plausibly replace this flag with
TargetFilter(see #7275 and #7283) if we had some kind of "language" aspect to the filter.There's some design work to do around how in v2 we'd pass correctly scoped versions of this around, as well as in v1 how we'd make this automatic (or if we'd just make this a breaking change and require manual opting in per-Task).
See https://pantsbuild.slack.com/archives/C0D7TNJHL/p1569513933176700 for more context
/cc @benjyw @stuhood
The text was updated successfully, but these errors were encountered: