Simplify configuration file search to be relative to cwd - [merged]

[asottile]

In GitLab by @ericvw on Oct 13, 2019, 13:42

Merges config-search-relative-to-cwd -> master

The starting point to search for the configuration file was based on the common prefix of the filenames positional arguments or . (i.e., the cwd) if no filenames were present. There is a strange situation where an invocation of flake8 outside of a project tree could end up with detecting a configuration file based on the common prefix shared by filenames provided as positional arguments.

To bring consistency towards how the configuration file is detected, regardless of the positional arguments provided, only search relative to the invocation of the current working directory of flake8. While the change is technically backward-incompatible, the aforementioned situation is most likely a rare occurrence. One could argue the current behavior is "buggy" because of the inconsistency 🤷.

Users impacted will either need to change their current working directory when invoking flake8 or pass the --config <path/to/config>.

Note that this change helps towards pre-configuration CLI parsing and layering two argparse.ArgumentParser objects with the parents parameter passed at construction.

It may be easiest to review each commit in-order to follow along with the approach I took for making this change. The goal I had was to ensure each commit, in isolation, passes tests and any other linting/documentation checks.

[asottile]

In GitLab by @asottile on Oct 14, 2019, 21:08

this one is a tricky one indeed, I do think it is the right direction but I want to make sure there's some discussion here first :) (sorry it took me so long to review this, I've been busy as usual)

my only hesitation is that it's fairly common for monorepo setups to invoke flake8 in setups that are atypical -- expecting that flake8 fooapp to pick up fooapp/setup.cfg

thoughts?

[asottile]

In GitLab by @ericvw on Oct 15, 2019, 14:23

this one is a tricky one indeed, I do think it is the right direction but I want to make sure there's some discussion here first :)

Completely understandable 😄.

(sorry it took me so long to review this, I've been busy as usual)

No worries — I completely get it.

my only hesitation is that it's fairly common for monorepo setups to invoke flake8 in setups that are atypical -- expecting that flake8 fooapp to pick up fooapp/setup.cfg

thoughts?

That's a good point... I didn't think of the monorepo case where one may desire this behavior. I can see the argument that by specifying positional arguments that the look-up binds closer to where the files are being evaluated.

The following tree gets interesting:

$ tree
.
├── fooapp1
│   └── setup.cfg
├── fooapp2
│   └── setup.cfg
└── .flake8

# What configuration is used/desired?
$ flake8 fooapp1 fooapp2
$ flake fooapp1
$ flake fooapp2

With or without .flake8 at the directory root, for flake8 fooapp1 fooapp2, if I understand the logic correctly, will resolve to the common prefix of . and not pickup either setup.cfg.

The sensitivity of the positional arguments and the configuration lookup is not what I would expect.

On the side of simplicity, using the current working directory is one path forward. On the side of more complexity, similar to how .gitignore files behave, bind separately the closes configuration file with each positional argument. Taking no sides is keeping things as is and not worrying about the change in behavior which may impact users.

Before I continue on my adventure of the pre-configuration CLI parsing, I need to know which way this merge request will go before I pivot/continue. I can live with whatever you (@asottile) and @sigmavirus24 are most comfortable with. Not needing to parse positional arguments before detecting and loading configuration makes layering the argument parsing logic a bit simpler, but everything is workable 😃.

[asottile]

In GitLab by @asottile on Oct 15, 2019, 15:39

just so we're clear, I'm +1 on approving + merging this, the current behaviour is undocumented and frankly difficult to explain.

as shown above, even the monorepo setup needs special consideration to split the commands in order to pick the right setup.cfg, it shouldn't be too much of an adjustment to add the --config or cd for the new version

[asottile]

In GitLab by @ericvw on Oct 15, 2019, 20:24

@asottile, awesome — we are definitely on the same page 👍.

Apologies for my writing style having a succinct/matter-of-fact tone. Let me take a step back here...

I am a new contributor to the flake8 project and am also sensitive to its reputation and usage within the Python community. When it comes to backward-incompatible changes, I wasn't sure what the preception would be proposing this merge request. I know some projects heavily favor backward-compatibility, where this type of change would be rejected.

My biggest fear is putting flake8's reputation on the line for changing this behavior. From an implementation point-of-view, it will influence the approach and path taken for handling pre-configuration CLI parsing.

So... it seems like we both are comfortable living with this change. Worst case scenario, we need to back it out and whatever other following changes that depend on this and I'll need to pivot the approach to the pre-configuration CLI parsing. Let's do this!

p.s., I am having a great time working on the project and greatly appreciate the time and effort you are spending on reviewing and discussing the changes I have been working on.

[asottile]

In GitLab by @asottile on Oct 23, 2019, 10:58

approved this merge request

[asottile]

In GitLab by @asottile on Oct 23, 2019, 10:58

mentioned in commit e2c4b50

[asottile]

In GitLab by @asottile on Oct 23, 2019, 10:58

merged

[asottile]

In GitLab by @sigmavirus24 on Apr 12, 2020, 13:12

So I'm coming to this rather late but I'm concerned about this. We've had people clamour for behaviour similar to eslint and I think with the way we do per-file-ignores we're really close to being able to just do that. This takes us in the opposite direction. That means having different style guides for fooapp1 and fooapp2 that know how to lint things based on their path. I still feel that having a config file in each sub-directory would only slow down flake8 and make the behaviour of the tool baffling to most folks.

On the other hand, if we break backwards compatibility with a thousand tiny paper cuts like this, then maybe eventually we can finally have manageable configuration handling and logic and even go so far as to support pyproject.toml.

[asottile]

In GitLab by @asottile on Apr 12, 2020, 13:27

This takes us in the opposite direction. That means having different style guides for fooapp1 and fooapp2 that know how to lint things based on their path. I still feel that having a config file in each sub-directory would only slow down flake8 and make the behaviour of the tool baffling to most folks.

I don't think that's quite the intention here -- before this didn't work: flake8 fooapp1 fooapp2 (the config machinery would try and find their unique path prefix, resort to . and ignore the config files at fooapp1/setup.cfg and fooapp2/setup.cfg), and it still doesn't work after this patch (now it doesn't attempt to find a unique prefix, just looks at .). Eric and I aren't really working towards supporting many-config-in-one-invocation -- we're just trying to fix the "relative paths aren't relative to the config" issue.

the handling of per-file-ignores needs some fixup and this is some of the work moving towards it -- notably it currently isn't handled relative to the configuration file, but instead handled relative to cwd.

the only break here is that flake8 fooapp1 by itself no longer will read flake8 fooapp1/setup.cfg

[asottile]

In GitLab by @pjacock on May 18, 2020, 14:32

Just to note that I had a unit test which broke with the v3.8 backward incompatible change, see peterjc/flake8-black#13 (comment)

Given the previous inconsistency (see also #561 and #562) this isn't unexpected. Relative paths in flake8 configuration files is probably best avoided for now...