john-hen / Flake8-pyproject

Flake8 plug-in loading the configuration from pyproject.toml
https://pypi.org/project/Flake8-pyproject
MIT License
227 stars 10 forks source link

Add prototype plugin #8

Closed vcalvert closed 2 years ago

vcalvert commented 2 years ago

It's rather difficult to modify flake8 configuration before plugins are loaded...at least as part of the standard configuration-loading process, so some monkeypatching was required...

Some work seems to be in progress on actually providing a plugin interface for configuration changes (see https://github.com/PyCQA/flake8/blob/main/src/flake8/options/config.py#L88, refactor ~9 months ago), but I couldn't find any GitHub issues tracking the actual addition of a flake8.configuration plugin type, so this uses flake8.extension (with a name of CFG99, which hopefully won't have any plugin name collisions).

I did test the initial plugin code with a simpler project, using setuptools, and it worked, but then I refactored to unify with what was here and add the plugin.

The updated code also traverses the filesystem to attempt to locate pyproject.toml in a parent directory, similar to flake8, though obviously it would have trouble with nested projects.

This PR should resolve #5 via autoconfiguring as a plugin, and should also address #7 due to the refactor / new find-file logic.

john-hen commented 2 years ago

Hi, thanks a lot for this PR! So it seems to be possible.

As you can see, the tests don't pass. In the Github Actions workflow, it cannot install the package as is (for some reason). Though when I ran the tests locally, skipping that installation step, there were also errors, such as this one:

  File "hook.py", line 23, in _read
    _, settings = load_flake8_from_toml(file)
ValueError: not enough values to unpack (expected 2, got 1)
vcalvert commented 2 years ago

I've found fixes for the build issue, including a syntax problem because I forgot which function I was using...and I did figure out how to run the tests...still fails on the config_flake8 / config_setup / config_tox tests, though, I think because the code now looks at parent directories first for the pyproject.toml...

I'll try to dig into it a bit later, but I suspect reverting the "look at parent directory" part would address that one...in which case how do we ensure the tests don't look at the parent? Technically overriding HOME (to the fixture root, for example) should do that, but it doesn't seem to work...

I'm probably going to try going back to the start, refactoring the existing hook to ensure all tests still pass, then adding the plugin, then adding the directory-traversal. This may require manually specifying --config <path-to->/pyproject.toml outside of the current directory but just the plugin would help...

@john-hen Maybe you can document the environment setup, or confirm that I'm doing the right thing for local testing? I'm using e.g.

pyenv virtualenv 3.8.6 flake8p-3.8.6
pyenv activate flake8p-3.8.6
pip install flit
flit install --symlink --deps develop
python ./tools/clean.py
python ./tools/wheel.py
pip install ./tools/dist/flake8_pyproject-1.0.0-py3-none-any.whl
python ./tools/test.py
john-hen commented 2 years ago

Hi Victor. So you get the test environment setup when you install with pip install .[test], or maybe install in "editable" mode, that's what I do: pip install --editable .[test]. Though the extra dependencies are simply pytest and the pytest-cov plug-in, and the latter is only needed to measure code coverage. Then calling pytest (or python -m pytest) in the project root folder runs the test suite.

Speaking of running things "in the project root folder"... I decided against implementing this "directory travsersal" that Flake8 does. Most tools, such as pyTest (see above), do not do that. Which is why I closed #7 as "won't fix". It's simpler that way and, I think, adds fairly little.

And yes, that's most likely why those tests fail. There is a way to solve this, but we'd have to replicate more of Flake8's internal logic. As we'd have to look for pyproject.toml and those other possible configuration files "at the same time", i.e. as we go up the directory tree.

So it's only about getting the hook to run as a plug-in when flake8 (and not flake8p) is called. As I noted in the related issue, #5, it didn't work when I tried. But if you get it to work, all the better.

vcalvert commented 2 years ago

Thanks for that information, John.

I removed the tree-traversal part, but kept the refactoring. I also had a slight bug in the entrypoint group name...oops.

Added tests to require the plugin, and then confirm the output.

It seems to be working properly now in both plugin and flake8p form.

john-hen commented 2 years ago

It works now, thanks a lot. Great job.

I will merge this now but will eventually refactor much of it before the next release. This installs a different hook when called as a plug-in, which works similarly to what I outlined in my comment in #5. So now we hook into parse_config(), instead of _find_config_file() and RawConfigParser before.

I see nothing wrong with this approach now that I tested your code. I was unsure before. (And also, didn't get this far to actually make it work.)

However, as it stands, we have both versions of the hook in the code base, each doing essentially the same. And when called via main, i.e. the flake8p command, both hooks are actually active. (Because we call Flake8, and Flake8 calls the plug-in.) There has to be a simpler way. And there is: I will just scrap the old hook. I already have a working draft that has fewer lines of code than the last release version. (It also helps to use a "report" plug-in instead of "extension".)

woile commented 2 years ago

This is awesome! Thanks ppl!