Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override Flake8's aggregate_options #20

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

mattbwilmes
Copy link
Contributor

@mattbwilmes mattbwilmes commented Mar 20, 2023

This PR addresses the concern brought up in #19

The issue was addressed by overriding Flake8's aggregate_options() function, similar to how Flake8's parse_config() is being overwritten currently.

This solution was seen as preferable to simply calling parse_known_args() because it allows this plug-in to work with calls to flake8.main.cli.main() (i.e. it doesn't assume that --toml_config is coming from the command line)

Testing

Wrote the following simple script to make sure it worked

import flake8.main.cli

flake8.main.cli.main(["--toml-config=repo/pyproject.toml", "test_file.py"])

Also ran the following command from my home directory to make sure it still worked

flake8 --toml-config=repo/pyproject.toml test_file.py

Confirmed that python3 tools/test.py still passes all tests

@john-hen
Copy link
Owner

Thanks for the PR!

As you can see, the test on Windows failed. That's because you assume there's an environment variable called HOME, which is not a given.

I also noticed that path names with a ~ in them are not properly interpreted and an error will occur claiming the file doesn't exist. This was resolved by replacing the ~ with os.getenv('HOME')

You'll have to remove that entirely. This is not supposed to work, not when doing this:

import flake8.main.cli

flake8.main.cli.main(["--toml-config=~/repo/pyproject.toml", "test_file.py"])

The expansion of the tilde is a feature of the Unix shell, which is not involved when you pass the arguments directly. It would work if you ran that command in the terminal. But replicating shell features is really not "our concern".

@mattbwilmes
Copy link
Contributor Author

The expansion of the tilde is a feature of the Unix shell, which is not involved when you pass the arguments directly. It would work if you ran that command in the terminal. But replicating shell features is really not "our concern".

Yikes, that was a really bad assumption on my part! I'm so used to working with code that is meant to run on Ubuntu that this hadn't crossed my mind... I'm glad there are tests in place to flag this type of stuff 💯

I pushed a change to remove this: f0a9e5d
And realized I forgot to remove import os (fixed in 34c658a)

@john-hen john-hen merged commit 182eb8e into john-hen:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants