Pants: Deprecate FmtTaskMixin's --skip option

[illicitonion]

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 https://github.com/pantsbuild/pants/pull/7275 and https://github.com/pantsbuild/pants/pull/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

[Eric-Arellano]

We could plausibly replace this flag with TargetFilter (see #7275 and #7283) if we had some kind of "language" aspect to the filter.

We need more than per-language, though. We need per-tool --skip. For example, Python has both isort and black for the fmt goal. It's not acceptable to say ~--fmt-skip-python, we need --fmt-skip-isort and --fmt-skip-black.

[stuhood]

I'm curious whether we will actually need --skip at all with v2... it's unclear that we will.

Some amount of research into "why people use --skip" would be good: if it's to avoid running tasks for speed reasons vs for "I don't wait to fail early" reasons, it's possible that it will no longer be necessary given concurrency and slow-failure of linters.

[Eric-Arellano]

I'm curious whether we will actually need --skip at all with v2... it's unclear that we will.

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 ./pants fmt-v2 :: we would now be running both isort and Black.

Likewise, users may not want to run tools like Scalafmt, Pylint, or MyPy.

[stuhood]

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.

[Eric-Arellano]

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 pants.ini:

[fmt.isort]
skip: True

[lint.isort]
skip: True

[Eric-Arellano]

Wait, the above got me thinking...We can simply set the --skip option on the subsystem itself! Notably, in V2, we are transitioning to having both a lint and fmt implementation for tools that support it, like Black and isort. (Some tools like Pylint will only have lint, of course).

It would be nice to simply say:

[isort]
skip: True

Or

[isort]
enabled: True

Rather than:

[isort.fmt]
skip: True

[isort.lint]
skip: True

I 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.

[benjyw]

Yeah, that's not bad. In that case, I prefer enabled: True. Linters/formatters seem like an opt-in kind of thing.

[stuhood]

True. Although, the second doesn't cover all situations because some formatters/linters are provided in core Pants, like isort and black.

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?

[Eric-Arellano]

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.

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 backend and into contrib packages. I don't think it's desirable for us to release all of these wheels to PyPI: pantsbuild.pants.contrib.python.pylint, pantsbuild.pants.contrib.python.isort, pantsbuild.pants.contrib.python.pylint, pantsbuild.pants.contrib.python.flake8, pantsbuild.pants.contrib.python.black, and pantsbuild.pants.contrib.python.bandit.

Instead, we could just release pantsbuild.pants.contrib.python and have users set --python-pylint-enable, --python-black-enable, etc.

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?

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.

[stuhood]

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.

[Eric-Arellano]

Opened https://github.com/pantsbuild/pants/pull/8672 to show what this proposal would look like for V2 Black.

[illicitonion]

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 :)

[benjyw]

I think multiple backends in a single plugin might work. So we publish a single pantsbuild.pants.python.lint plugin, that contains register.py files for each linter, and you have to include each one in backends. We do this today for codegen: we have 10 register.py files (under pants.backend.codegen.antlr.java, pants.backend.codegen.antlr.python, pants.backend.codegen.jaxb, pants.backend.codegen.protobuf.java, and so on) and you have to opt in to just the generators you want, by specifying them in backends. These are in core pants and not a plugin only for historical reasons.

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.

[benjyw]

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 backend, possibly including what's currently under rules/core. And several backends can get published as a single plugin. And basically you should have to opt-in to anything you want to use, nothing (other than a handful of core rules) is shipped by default.

[stuhood]

Relates to #8817.