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

BLD: ignore pep8 E302 (expected 2 blank lines, found 1) #3661

Merged
merged 1 commit into from
May 17, 2014

Conversation

argriffing
Copy link
Contributor

Several recent scipy PRs by new contributors have been denied by this pep8 rule. Perhaps we could keep the pep8 bot, but add this rule to the ignore list? This is not to suggest not following this pep8 rule in scipy code. And I understand that the change in this PR is controversial, so I don't necessarily expect it to be merged.

@WarrenWeckesser
Copy link
Member

This is a pretty simple rule to follow: http://legacy.python.org/dev/peps/pep-0008/#blank-lines
I suspect the problem is that new contributors simply aren't aware of this part of PEP8. So the pep8-bot is doing its job. :)

@rgommers
Copy link
Member

I'm OK with turning this one off. It doesn't bother me too much if it's not followed, and having the bot make Alex' day bad isn't good for anyone:)

@rgommers rgommers added the PR label May 17, 2014
pv added a commit that referenced this pull request May 17, 2014
BLD: ignore pep8 E302 (expected 2 blank lines, found 1)
@pv pv merged commit 37cc947 into scipy:master May 17, 2014
@WarrenWeckesser
Copy link
Member

@argriffing wrote:

This is not to suggest not following this pep8 rule in scipy code.

If that is the case, then it is a bad idea to ignore these PEP8 violations in pull requests. A pull request is the best time to ensure that PEP8 is followed. We should expect a contributor to be familiar with PEP8, and to be able to run the pep8 program on their code before they submit it. Otherwise, when does it get fixed? Answer: in many cases, it won't.

On the other hand, if we don't care about following this rule in scipy code, then we should just 'fess up and admit it. In that case, we should merge this PR. In effect, our policy on this rule becomes "We're not against it, but it is not required."

Personally, I prefer to keep the PEP8 exceptions to a minimum, if only to avoid potentially long discussions about style questions that already have reasonable answers in the PEP8 guidelines. PEP8 isn't perfect, but it's not too bad, and it already exists, so we can get on with writing, fixing and reviewing code.

@pv
Copy link
Member

pv commented May 17, 2014

No real objections from me on ignoring this check. But I agree with Warren that merging this PR implies that the rule will not be followed in the code base. I know I fail to follow it in code I write myself without automatic tools to check it.

One of the issues in running the check in travis is that the comments the robocop makes do not appear directly in the PR, so that reviewer time is wasted in pointing them out to contributors (overall, I think that manual whitespace review should not be done in the PRs, and should be automated). There are alternatives to checking pep8 in travis, e.g. https://github.com/markstory/lint-review that remove that need. Setting those up however takes up some effort, which might be better spent elsewhere, however.

@pv pv added this to the 0.15.0 milestone Jun 10, 2014
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

5 participants