#2436 Fix pep8 warnings in pagure/ folder
Closed by pingou. Opened by stbuehler.
stbuehler/pagure fix-pep8-warnings  into  master

Download 2436.patch

Mostly W503 (line break before binary operator), and in comments E265 (block comment should start with '# '), E261 (at least two spaces before inline comment) and E501 (line too long).

Also dropped some '# noqa' markers that didn't silence a warning.

git show --color-words shows that mostly only whitespace got moved around.

Maybe you want to adept the contribution hints that pep8 only applies to the pagure/ folder?

W503 (line break before binary operator)

This one I actually want to keep as is and pep8 itself allows it (just the pep8 tool doesn't, but the pep8 tool has also made some decision that the pep8 specs leave open) https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

I don't like the "W503 approved style" either, but don't you see the irony requesting contributors to use the tool to check for it, but not actually wanting it?

Combine that with the fact that the unit tests don't work when following the instructions in README.rst... (it probably requires tons of dependencies and daemons that I have no intention to setup on my system)

You were pissed in PR#2426 that I didn't run pep8 nor unit tests, but I'm REALLY pissed that you ask this from contributors.

I'm going to get my patches in a shape I think is good enough, and will open some pull requests (but certainly not individual ones) - it will be your decision what to pick, and how to clean them up. If you want "better" pull requests, you gonna have to do some work yourself first.

but don't you see the irony requesting contributors to use the tool to check for it, but not actually wanting it?

You are right that I should change this template, I've been making the pagure sources flake8 valid as this tool is more aligned with the coding style I want.

Thanks for pointing this out.

As for the unit-tests, to be honest I have not follow the README in a new system in a while, things have changed quite a bit with 3.0 (which I basically just out), I am on PTO this week (yet still replying to you) and the documentation can be/is likely outdated.

If you run into this kinds of issues, feel free to open tickets, we can makr them easyfix and let someone handle them.

As for #2426 I wasn't really pissed, I was on pto, saw your PR, looked at the code which I considered good but your comment wasn't about not being able to run the tests locally it was worded in such a way that I felt you didn't even bother with them, so I felt I wouldn't bother with this PR until I got back from time AFK.

Also, both flake8 and pep8 (the tools) have a --diff option which allows you to run the tool against just your diff, without bothering about errors outside of your changes, quite handy :)

I should be back next week, I will run the test locally for #2426 and will most likely end up merging it or give you more/better feedbacks.

Thanks for your work on pagure and sorry we got off on the wrong foot :(

I should change this template,

I just did that, it now refers to flake8 and even precises that flake8 is run in the test suite.

I'm sorry that I didn't get around to polish up my changes; for the record I have some fixes in https://pagure.io/fork/stbuehler/pagure/commits/fixes

@stbuehler I took some of your patch and included them in https://pagure.io/pagure/pull-request/2781

Some of the others no longer apply and one I've to think more about :)

Thanks for them!

I'm going to close this PR though, the PR template has been adjusted to speak about flake8 and the test suite actually ensures it now :)

Pull-Request has been closed by pingou

Metadata