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

Add flake8-docstrings to test_requirements #138

Closed
wants to merge 1 commit into from

Conversation

mdellweg
Copy link
Member

When running flake8 linting without this package, flake reports less
errors than the automatic ci pipeline. Putting i here will ensure, that
is is installed on all pulplift development boxes.

[noissue]

When running flake8 linting without this package, flake reports less
errors than the automatic ci pipeline. Putting i here will ensure, that
is is installed on all pulplift development boxes.

[noissue]
@mdellweg
Copy link
Member Author

Hmm, seems, like i was wrong. But it was definitely true for deb plugin, where the docstrings are linted.

@bmbouter
Copy link
Member

@mdellweg so what should we do with this PR?

@mdellweg
Copy link
Member Author

Good question. For starters, i think it is odd, that the enforcing of some linting rules is solely dependent on the existence of a package in the system. But that is the part we cannot control.

On the other side, I think it would be beneficial to follow the same linting policy throughout the whole project. Being it with or without docstring linting should be up to discussion.

The third thing the original problem shows, is that we should install dev&test requirements of plugins in the source boxes of pulplift. I can have a look into that.

@nixocio
Copy link

nixocio commented May 21, 2019

For pulp-smash we used to have a dev section on setup.py with all linters, and a Makefile to call those linters.

https://github.com/PulpQE/pulp-smash/blob/f14a3d7bd391761c5f22bd908555b22734c6e261/setup.py#L51

A time ago we change to pre-commit with pre-commit hooks - pre-commit will install the packages for the required hooks in a separate env and run them. No need to add the package as dependency. There are a lot of pre-commit hooks available.

See: pulp/pulp-smash@34ac7e2

@bmbouter
Copy link
Member

@dkliban this thread has some of the linting ideas that we may want to incorporate into the managed CI tooling.

@bmbouter
Copy link
Member

@mdellweg Travis fails with ~800 linting errors with this type of linting enabled. If we are to merge this we'll have to have all that done. What do you want to do? I'm ok w/ whatever you want (or don't want) to do.

@mdellweg
Copy link
Member Author

@bmbouter That depends on whether we want to lint those docstrings or not. But we can close the ticket here an take the discussion to the mailinglist if you prefer.

@bmbouter
Copy link
Member

@mdellweg I'm +1 on us linting those docstrings too. Those become part of our docs and if we can increase consistency that would be a good thing. Some mailing list discussion would be good since AIUI this change would show up with new Travis errors in any CI pipeline that uses pulpcore (which is all of them). Is that right?

@mdellweg
Copy link
Member Author

I am not sure. In pulp_deb and pulp_ansible at least they are already active in the travis pipeline (that was my original problem to understand why they did not pop up locally).
But with the new pipelines you mentioned i have even less of an idea.

@bmbouter
Copy link
Member

I see. Well then it's probably safe to add here if someone is willing to make all the changes. The big win I hope for is for us to add this type of linting to the managed CI pipeline @dkliban is starting.

@mdellweg
Copy link
Member Author

@bmbouter Moved the discussion to the mailinglist. Shall we close this one?

@bmbouter
Copy link
Member

+1 to closing this for now, and we can bring it back w/ a plan to resolve the epic linting issues it discovers.

@mdellweg mdellweg closed this May 21, 2019
@mdellweg mdellweg deleted the test_install_flake8-docstrings branch July 22, 2019 19:35
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.

None yet

3 participants