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

a way to add to sys.path so local plugins can be found #1067

Closed
asottile opened this issue Apr 3, 2021 · 9 comments
Closed

a way to add to sys.path so local plugins can be found #1067

asottile opened this issue Apr 3, 2021 · 9 comments

Comments

@asottile
Copy link
Member

asottile commented Apr 3, 2021

In GitLab by @carljm on Oct 16, 2017, 09:36

In trying to use the new local-plugins support, we've realized that setting up sys.path so flake8 can find the local plugin is a bit of a pain point. It's solvable with PYTHONPATH, but it's a bit sad to require that to be set for every flake8 run. It would be great to be able to set things up with a local plugin such that simply running flake8 works.

Some possible ideas:

  1. If a local-plugins section is found in a config file, add that config file's path to sys.path. This is simple and pretty intuitive, though it lacks flexibility for situations where you might have an intervening src/ dir in between the flake8 config and the actual source, and you want your local plugin with the actual source (e.g. so it can be tested), not next to the flake8 config.

  2. For more flexibility, add a sys-path or local-plugins-path option, which could be set to e.g. . or ./src (relative to the config file location).

I'd lean toward (2) -- probably named local-plugin-path or similar. Interested in your thoughts before I submit a merge request. Thanks!

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @carljm on Oct 16, 2017, 09:37

Ugh I messed up markdown numbered-list, and gitlab is erroring on updating the issue. Hopefully it's still clear; option (2) is just labeled as 1. :-)

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Oct 17, 2017, 04:42

changed the description

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Oct 17, 2017, 04:58

Let me see if I can tease out what is going on, because I feel like there's more than meets the eye with this proposal.

There's hypothetically a repository that has most of its code in src including the local Flake8 plugins. For example, you have stuff like

  • src/application
  • src/local_plugin0
  • src/local_plugin1
  • src/local_plugin2
  • etc.

And your config file has:

[flake8:local-plugins]
extension =
  p0 = local_plugin0:ExtensionPlugin0,
  p1 = local_plugin1:ExtensionPlugin1
report =
  p2 = local_plugin2:ExtensionPlugin2

And you're running Flake8 right now in such a way that we can't find those local plugins (and you're on a version of Flake8 that supports them).

I think the confusion in the original design lies in these parts: When I think of how projects run tests, linters, etc. they do so in a virtualenv with everything installed that's in src. So in that case, local_plugin* would be installed in the virtualenv and Flake8 would be able to import local_plugin0 effectively and run your local plugins. That usage pattern is supported/exemplified by tools like tox and pipenv.

The ask here is that you not be forced to do that such that Flake8 modifies the path so that import local_plugin0 Just Works. I'm biased against that because if people decide to depend on something in their plugin (that may or may not be a dependency of their application code) they now have to ensure it is installed on their own. I assumed it would be something they can shove into (for example) requirements.test.txt.

Likewise, I had (in my head when we were designing this) the assumption that the layout would be more like:

  • src/application/
  • src/application/local_plugin0
  • src/application/local_plugin1
  • etc.

Which would mean that the local plugins are installed by virtue of installing the application. Therefore no need for extra setup.py shenanigans to register the plugin with Flake8 or anything.

Is this where our communication broke down?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @carljm on Oct 17, 2017, 09:11

You're totally correct, this problem is also solved by making the application itself an installable package, and installing it in the Python environment you're running flake8 from. In my experience, when working on a deployable web application, making it an installable package is not a universal practice. Unfortunately it's not really an option in my current situation; we install and run flake8 from an RPM, not via pip requirements, and it doesn't run in the application virtualenv.

If you feel that making the application installable and running flake8 from the application's execution environment, with the application installed in it, is the universal best practice and you don't see the need to support a different workflow, I respect that; we'll use PYTHONPATH workarounds instead. Really the best case I can make for this feature is that it would be useful to us, and the implementation is very simple, so it shouldn't bother anyone else too much :) Your call.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Oct 19, 2017, 05:26

If you feel that making the application installable and running flake8 from the application's execution environment, with the application installed in it, is the universal best practice and you don't see the need to support a different workflow, I respect that;

Sorry, that's not what I was trying to say. I understand full-well that installing an application locally isn't a universal practice. It's where I was coming from (primarily, OpenStack and F/OSS development).

My preference is that we document clearly the case where the option is necessary because otherwise, I suspect people will try to use it when they don't need it. That's my concern. I also wanted to explain why it wouldn't have otherwise seemed like an issue to me.

A pull request implementing this would be great. I think, it would make sense to put the setting in the same place as where we define the local plugin extensions to make the relationship very clear. Does that make sense to you as well?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @carljm on Oct 23, 2017, 17:04

mentioned in merge request !211

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @carljm on Oct 23, 2017, 17:07

Awesome! Totally agree with you that we should be clear about when this is and isn't needed, so people don't think they need it when they don't. I tried to do this in my documentation in !211, let me know what you think. Thanks!

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 26, 2017, 10:40

closed via commit 3530870

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 26, 2017, 10:40

closed via merge request !211

@asottile asottile closed this as completed Apr 3, 2021
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

No branches or pull requests

1 participant