Open Bug 1499115 Opened 6 years ago Updated 3 months ago

Editor integrations for Black frustrate devs and train them to ignore problems

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

enhancement

Tracking

(Not tracked)

People

(Reporter: dmosedale, Unassigned)

References

Details

Because we specify which flake8 directories are included (and maybe eventually excluded, as per bug 1367092) in a .yaml file, rather than in the .flake8 file itself, editor integrations assume that they should be linting the entire tree, which has a couple of bad consequences:

* unlinted files tend to violate the lint rules in a various, leaving them splattered with red markers in the editor that devs initially get distracted by, until...
* they get (partially?) trained to ignore the markers, which results in them being ignored in places where they are used and then flagged much later in the process, slowing down the feedback loop, possibly even causing backouts.
Yes, I agree this is pretty frustrating. I don't really know if moving paths out of the .yml would be feasible or not (I think we'd run into trouble when editing files that have a subdir .flake8 applied to them). I agree it would be a good idea to block on bug 1367092 as the landscape would be a lot simpler then.

I think using |mach lint| is the only way we'll be able to lint things exactly the same way that CI does, so IMO figuring out how to get |mach lint| itself integrated into the editors we care about should be a higher priority. (I realize we'll never have an integration for every editor that also has a flake8 integration, so this bug will always be a problem for some)
Interestingly, I just tried to make VS Code run "mach lint -l flake8", and the settings I added to the workspace were:

    "python.linting.flake8Args": ["lint", "-l", "flake8"],
    "python.linting.flake8Enabled": true,
    "python.linting.flake8Path": "/Users/dmose/r/git-cin-mc/mach",
    "python.linting.enabled": true,

Which I would expect to work, but instead, it seems to silently fail (or maybe run lint on /dev/null or something without errors.  Doh!
I'm betting it's expecting the output to be in a format (the default |mach lint| format is what ESLint uses). You can use |mach lint -l flake8 -f unix| to output a more standard and parse-able format, though it still might not be what VS Code is expecting.

If it turns out it is a format problem and you do figure out what it expects, we can create a new formatter that outputs what you need.
Actually, the 'unix' formatter is very close to flake8's default.. it *might* just work.
So, excitingly, VS Code doesn't use the default format.  It expects the flake8 executable to be able to handle a specific format string, which, at the moment, mach doesn't:

https://github.com/Microsoft/vscode-python/blob/master/src/client/linters/flake8.ts#L16
Rats. Ideally it would be great to find a way to get this hooked up in a non-python specific way (and let mozlint determine what linters to run a on given file). There may be an extension that lets us define custom linters (e.g command + output format), or maybe in the worst case we'd have to make our own extension.
(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Yes, I agree this is pretty frustrating. I don't really know if moving paths
> out of the .yml would be feasible or not (I think we'd run into trouble when
> editing files that have a subdir .flake8 applied to them). I agree it would
> be a good idea to block on bug 1367092 as the landscape would be a lot
> simpler then.

Rather than doing this, maybe we could do something like the ideas in bug 1366714 and have the ignore files generate automatically by the build? 

> I think using |mach lint| is the only way we'll be able to lint things
> exactly the same way that CI does, so IMO figuring out how to get |mach
> lint| itself integrated into the editors we care about should be a higher
> priority. (I realize we'll never have an integration for every editor that
> also has a flake8 integration, so this bug will always be a problem for some)

I'm cautious about this approach, and it might need wider discussion. There's a lot of plugins that already do the same functionality for us for the individual linters - taking care of line highlighting, providing links to documentation, automatic fix options - and loosing this means we'd need to commit to maintaining our own versions of those plugins long-term.

That's one of the reasons I've always been an advocate of allowing linters to work in their native fashion - so that we can get editor integrations for free. ./mach lint is something I rarely use except for when I change configuration of the entire tree, (or when it gets run behind the scenes for hooks).
(In reply to Mark Banner (:standard8) from comment #7)
> I'm cautious about this approach, and it might need wider discussion.
> There's a lot of plugins that already do the same functionality for us for
> the individual linters - taking care of line highlighting, providing links
> to documentation, automatic fix options - and loosing this means we'd need
> to commit to maintaining our own versions of those plugins long-term.
> 
> That's one of the reasons I've always been an advocate of allowing linters
> to work in their native fashion - so that we can get editor integrations for
> free. ./mach lint is something I rarely use except for when I change
> configuration of the entire tree, (or when it gets run behind the scenes for
> hooks).

This works for popular and well designed linters (like eslint), but in my experience most linters are woefully inadequate for mozilla-central's use case. They tend to be designed with a single (smallish) repo in mind and aren't equipped to handle the complexities of a monorepo as large as ours. E.g flake8's configuration mechanism doesn't support different configs for different files. That's something we either have to implement ourselves or we need to apply the exact same configuration everywhere (which sounds fine in theory, but there are lots of edge cases in practice). And this doesn't even take into account our many custom linters that don't have integrations in the first place.

All that is to say that I do agree that we should aim to set things up such that linters can run natively with as close to the same config as possible. But I think even if we do, we'll *still* want |mach lint| editor integrations (devs can always choose whether they want to use a native integration, it's just that if they do, we can't guarantee that it'll catch the exact same errors). Fwiw, I really don't want to write big complicated plugins. My hope was that editors have some kind of pre-existing way to define a lint program + errorformat. E.g, here is how I integrate |mach lint| in my vim:
https://github.com/ahal/configs/blob/master/nvim/init.vim#L99

My hope is that we can get away with similarly easy ways to provide integrations in many cases.
One more benefit of specifying paths in `mozlint` rather than the native linters is parallelization. Not only does mozlint run each linter in its own subprocess, it also breaks down specific linters by paths. If we handle all path filtering in the native linter, we lose that parallelization (unless it happens to have its own built-in parallelization, which most seem not to have).
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> This works for popular and well designed linters (like eslint), but in my
> experience most linters are woefully inadequate for mozilla-central's use
> case. 

The general argument here is good, I can understand where you're coming from now.

> They tend to be designed with a single (smallish) repo in mind and
> aren't equipped to handle the complexities of a monorepo as large as ours.
> E.g flake8's configuration mechanism doesn't support different configs for
> different files. That's something we either have to implement ourselves or
> we need to apply the exact same configuration everywhere (which sounds fine
> in theory, but there are lots of edge cases in practice).

FWIW I think we should fight against these, and try to enforce more consistency across projects within mozilla-central. These are just standards, we shouldn't be inventing new ones for each bit of code.

That said, I'm aware that ESLint isn't exactly consistent, but generally the rules do have the same settings and other directories tend to build on the core set (if I could, I'd just roll them all out everywhere today).
(In reply to Mark Banner (:standard8) from comment #10)
> FWIW I think we should fight against these, and try to enforce more
> consistency across projects within mozilla-central. These are just
> standards, we shouldn't be inventing new ones for each bit of code.
> 
> That said, I'm aware that ESLint isn't exactly consistent, but generally the
> rules do have the same settings and other directories tend to build on the
> core set (if I could, I'd just roll them all out everywhere today).

I do agree with this, and if I could I would also roll out a consistent flake8 config. But python is a bit of a unique case due to the 2/3 divide. We currently have code in-tree that is py2 only, py3 only and both. Right now they all use the same config, but as we migrate a larger percentage of code over, we'll want to start enabling rules on our new py3 code that don't affect the old legacy stuff.

I think we mostly agree on things, the main disagreement seems to be a prioritization one (do we spend time improving the native linter experience vs spend time improving |mach lint|).
Priority: -- → P2

Just noting that bug 1367092 will remove the inclusions from flake8.yml, and add the new exclusions to the root .flake8, which should improve the situation here a lot. Subdirectory .flake8 files were also removed recently which should further consolidate the editor vs mach configs.

It still won't be perfect though, mostly due to the run time exclusions mach adds. These are mostly just third party paths and objdirs though, so I suspect editing + linting those kinds of files will be pretty rare anyway.

Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3

Flake8 has been replaced by Ruff/Black, Ruff looks ok now as there's a top-level config, but I think this still applies in the Black case.

Summary: editor flake8 integrations frustrate devs and train them to ignore problems → Editor integrations for Black frustrate devs and train them to ignore problems
You need to log in before you can comment on or make changes to this bug.