Recursively update configuration as we traverse subdirectories

[asottile]

In GitLab by @ahal on Jun 3, 2016, 09:16

This is a pretty big change, but maybe something to consider either for 3.0 or post 3.0. Currently (both 2.x and 3.0a) finalize the configuration at the start, and then apply that same configuration to every file that gets linted. I think it makes more sense to instead scan for configuration at each directory as we go along.

So starting at the project root (or cwd):

1. Load configuation in directory (overriding any existing config)
2. Lint files in directory
3. Move to included subdirectory
4. Go back to 1

This allows you to have a global project configuration, but also allow sub-directories within that project to set their own rules.

[asottile]

In GitLab by @ahal on Jun 3, 2016, 09:19

I meant to add, this is what eslint does, and it works really well. Here is how they describe it:
http://eslint.org/docs/user-guide/configuring#configuration-cascading-and-hierarchy

Though eslint works backwards. So they start at the file being linted, then scan parent directories until they come across a "root: true" config. This could be an alternative to the recursive route.

[asottile]

In GitLab by @ahal on Jun 30, 2016, 14:39

I did a bit of digging and this would need a fairly substantial refactor. First, option parsing would need to happen later on, not at the beginning. Likely somewhere around "make_checkers()" in checker.py. This is so each directory has a chance to walk its ancestors searching for config files along the way. This means we'd have to delay instantiation of things like the style_guide. This isn't too hard to do, but the problem is that some config options do need to get parsed at the beginning. Things like --jobs and --statistics that are related to a run of flake8 rather than style options. So now we'd need a separate type of configuration called "run_options" that get parsed at the beginning, while "style_options" get parsed later on.

Anyway, it's likely a lot of work but looks do-able. I'm still not sure if I'm going to work on this or not, but it's a fairly fundamental shift in how flake8 works so I'd want some sort of approval before putting in any more time. For context, I'm setting up some lint infrastructure at Mozilla and we need per-directory configuration (project wide is not granular enough). I'm currently working around this limitation in flake8 by spawning a separate flake8 process per directory. I'd obviously prefer not to do that.

@sigmavirus24 is this a change you'd accept if I decided to work on it? Not making any promises even if it is ;)

[asottile]

In GitLab by @sigmavirus24 on Jun 30, 2016, 17:21

So I've been trying to decide on that @ahal.

In general, I'm not a fan of per-subdirectory config files and let me explain why. If I contribute code in dir1/subdir1 and see that E241 is enforced there, I'm going to be really confused if it's not enforced in dir1/subdir2/ or even dir2/.... Eventually, I'd find the config files in each directory, but until then it would seem to me like Flake8 is broken. Frankly, I think projects should have style guides. These are usually documented in the documentation for contributors. Having per-subdirectory style guides means you can't document the style or reasoning for contributors in one place. This also severely complicates what happens when you have a config file in each directory and someone who specified --select and/or --ignore on the command-line. Does that override every config file? I don't know, it seems nebulous.

I understand that adding flake8 to an existing code-base can be really challenging with this in mind. That said, there are plugins that kind of do what you want (the first that comes to mind is flake8-putty).

I'd probably need to digest all of this and how it would work and how different command-line flags would affect it. So it might not be a 3.0 feature, no, but I'm also not keen on rejecting it just yet. It's something that has been requested before and it's distinctly more possible with the 3.x branch of Flake8.

[asottile]

In GitLab by @ahal on Jun 30, 2016, 17:47

Thanks for sharing your concerns @sigmavirus24, I do sympathize with them. I'll hold off doing anything here for the time being and fall back to multiple flake8 invocations for now.

Here's a bit more info about my use case. At Mozilla we have a monorepo (every project/library/file is checked into the same giant repository). I do agree with you that configuration should be the same project-wide. The problem is that you're assuming that you'd only ever want to run flake8 from the root of a project, where I want to run it from the root of a repository that contains hundreds of projects. And I can't enforce a global style across all these different projects (which all have different owners). The system I'm implementing will be designed to let the owners of each project (aka subdirectory) choose the style that suits them best.

I'd also argue that if we did have subdirectory configuration, project owners would still be able to disallow subdirectory .flake8 files in their repos if they want. Finally as for the question of precedence, I'd suggest in order of most to least precedence: commandline configs > subdirectory configs > parent directory configs

I really do like flake8 and want to thank you for all the work you do maintaining it.

[asottile]

In GitLab by @sigmavirus24 on Jul 1, 2016, 07:46

Also, of note, if you do independent flake8 invocations, something like:

flake8 path/to/project

I think the current behaviour (and the behaviour on Flake8 3) is to look for config files in each of

• path/to/project
• path/to
• path
• ./

And then read them in that order (which means the one closest to your code loses), e.g.,

• path/to/project/config
• path/to/config
• path/config
• config

Where the top-level one takes precedence. Given how soon I want to release 3.0 final, I think we could fix this just there if you think the order of importance should be reversed (which I'm fine with and think makes sense). That probably happens in flake8.options.config in the finder object.

Let me know your thoughts.

And thank you for sharing your use case. I keep forgetting those monolithic repositories with tons of projects are things.

[asottile]

In GitLab by @ahal on Jan 16, 2017, 08:54

Sorry for the long delay between replies, I missed your comment. I see multiple version of flake8 3 have been released since and I haven't checked to see if or how configuration has changed in the meantime. So please correct me if I'm working on faulty assumptions now.

I think reversing the order of precedence like you suggested would be a big improvement. However, it's still not as convenient as updating the configuration as we go, because if sub-projects only want to change one or two things from the global defaults, they would need to re-define the entire global config. But this isn't a huge deal, I'd be happy with simply reversing the order of precedence if that's easier.

[asottile]

In GitLab by @sigmavirus24 on Jan 16, 2017, 10:34

@ahal I did not make any changes. Flake8 3.x broke enough things in one release and I didn't want to completely pull the rug out from every user that had been using Flake8 previously. With that in mind, and the fact that I don't think Flake8 will ever reach 4.0 (or if it does, will not have so many - if any - breaking changes) I'm not sure this change will be plausible. It represents a severe break from the status quo of the last several years. Further, provided the significant amount of refactoring necessary, it sounds like it might disturb things and cause more instability than I'd like to see in Flake8 moving forward.

[asottile]

In GitLab by @ahal on Jun 15, 2017, 14:36

@sigmavirus24 would you be amenable to a new "aggregator" plugin type that can replace the default flake8.options.aggregator.aggregate_options function?

That way it would be possible to overhaul the config mechanism with a plugin (built-in or otherwise) without breaking backwards compatibility. If a version 4 ever did roll out, the default aggregator could be easily swapped to the newer one if desired.

[asottile]

In GitLab by @ahal on Jun 15, 2017, 19:49

Or actually, a way to override the file_checker_manager would be more useful. Then a per-file configuration system like eslint has would be possible.

[asottile]

In GitLab by @sigmavirus24 on Oct 20, 2018, 10:41

I would prefer #156

[asottile]

In GitLab by @sigmavirus24 on Oct 20, 2018, 10:41

closed

[asottile]

In GitLab by @asottile on Oct 2, 2020, 16:38

changed the description