loadsmart / danger-pep8

A Danger plugin for Python's PEP 8
https://www.python.org/dev/peps/pep-0008/
MIT License
9 stars 3 forks source link

Support for flake8 plugins #7

Open danpalmer opened 7 years ago

danpalmer commented 7 years ago

We currently use ~5-10 flake8 plugins to customise our flake8 linting behaviour. Given that flake8 puts such an emphasis on plugins, would this be a feature that danger-pep8 would accept?

I envision the API being either:

I think the latter would be a more flexible implementation that would fit into the Python ecosystem better, but the former may be a nicer Danger/Ruby API. Thoughts?

barbosa commented 7 years ago

Given the discussion on #3, this sounds like a good thing to be implemented.

In general, I would prefer the first option, but it might create some unwanted duplication (Dangerfile + requirements.txt), painful to maintain.

On the other hand, the latter would install every single lib from requirements.txt. It can be solved with 2 different files though, or maybe if all flake8 plugins start with flake8, this Danger plugin can filter the others out.

danpalmer commented 7 years ago

For clarity, working on Python projects, we already have a 'requirements-linting.txt' that we integrate with other tooling (like flake8 editor plugins), so for us, the list in the Dangerfile would be added duplication.

I'm strongly in favour of using a requirements file, or at least having that as a possibility, but I also know very little about the Ruby/Danger ecosystem so don't want to 'demand' it in case that is really the wrong architecture here.

Another thing that might be worth it is using a virualenv. When installing lots of python packages it could clutter up the global environment, unless Danger is run under Docker or something. Is this worth a separate discussion on a new PR?

rcmachado commented 7 years ago

@barbosa As not all plugins starts with flake8-* (eg. https://pypi.org/project/pi-naming/), trying to filter them would not be guaranteed. In that scenario, we would have another way to specify the plugins anyway.

On the other side, @danpalmer idea of using a separate requirements file for flake8 stuff could eliminate the need of manually filtering each entry. Obviously, users can abuse this and pass a generic requirements file - but given that Danger normally runs after you installed your dependencies this wouldn't cause errors. Do you have any API design in mind, @danpalmer ?

About using a virtualenv: not so sure. When Danger runs, you already have a virtualenv configured for your project (or you're using Docker, as you mentioned). Trying to manage another virtualenv in that situation could not worth the trouble. But as this is a separate discussion, feel free to open an issue if you want and we can discuss more there :)


Edited: fixed mention to Gustavo Barbosa :)