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

CI: Use the latest pep8speaks and remove custom configuration #25605

Closed
datapythonista opened this issue Mar 8, 2019 · 2 comments · Fixed by #26226
Closed

CI: Use the latest pep8speaks and remove custom configuration #25605

datapythonista opened this issue Mar 8, 2019 · 2 comments · Fixed by #26226
Labels
CI Continuous Integration good first issue
Milestone

Comments

@datapythonista
Copy link
Member

Until now, pep8speaks used independent settings, and not the setuptools standard setup.cfg. This has been addressed in the latest version of pep8speaks, and we shouldn't need to repeat our configuration anymore: https://github.com/pandas-dev/pandas/blob/master/.pep8speaks.yml#L10

More info about the problem can be found here: pep8speaks-org/pep8speaks#95 (comment)

Not sure how pep8speaks is integrated, but we should upgrade to the latest version if it's not automatic, and we should remove the duplicated configuration in .pep8speaks.yml (which is already present in setup.cfg).

@OrkoHunter
Copy link

Not sure how pep8speaks is integrated, but we should upgrade to the latest version if it's not automatic

The update is automatic with GitHub app, hence no upgrade is required. This issue can be solved by just removing the duplicate from .pep8speaks.yml.

@alimcmaster1
Copy link
Member

Hey @datapythonista , I was thinking we could actually just remove all warnings/errors we ignore from .pep8speaks.yml. As I imagine that most of the stuff we ignore in setup.cfg/.pep8speaks is most likely due to legacy reasons opposed to a pep8 rule we don't care about.

That way when reviewing code we can see that we are not increasing the occurrences of any particular warnings/errors. Say for example if I increase occurrences of E731 it is down to the reviewer to spot this?

As if setup.cfg and pep8speaks.yml are kept consisting a change can increase occurrences of any of these errors without anything highlighting so on the PR. We can keep diff_only: True set as per than current set up. I personally think there is more value in this, as otherwise pep8speaks and flake8 running in CI are just telling us the same thing. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants