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

WIP: Clean up trailing whitespace in PEP sources #1022

Closed
wants to merge 3 commits into from

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented May 6, 2019

Fixes #988

This is a WIP but I welcome feedback on the code changes here.

The main thing left is that PEP 0 is generated with trailing spaces. I can think of two approaches to deal with that:

  • Add a flag to pep2html.py to relax the constraint.
  • Modify genpepindex.py to not generate PEP 0 with trailing spaces.

I'm leaning toward doing the latter.

@pradyunsg
Copy link
Member Author

pradyunsg commented May 6, 2019

Also, there is a weird character in the generated output and some of the files. For now, my changes trim them from the trailing portion but not the start of lines.

(image below)

Screenshot 2019-05-06 at 6 37 18 AM

@pradyunsg pradyunsg changed the title Clean up trailing whitespace in PEP sources WIP: Clean up trailing whitespace in PEP sources May 6, 2019
@gvanrossum
Copy link
Member

Other than satisfying your desire to "Marie Kondo" the world, what problem does this solve?

@pradyunsg
Copy link
Member Author

pradyunsg commented May 7, 2019

@gvanrossum the idea comes from your review comment on a similar PR merged by you, where it was suggested to do this: #983 (review).

This PR adds the automation to prevent this from happening again (unlike the merged PR) and cleans up all the remaining instances of trailing spaces so that the build works.

If you've changed your mind about this topic and this is not welcome, that's fine by me. Please do close this PR and #988 in that case.


Please don't assume or imply things about what motivates me to contribute here or elsewhere. I'm just trying to help out and this seemed like a trivial change to make.

I don't have any desire to '"Marie Kondo" the world' and your implication that I do, is dismissive and feels more like a personal criticism of me based on an assumption, possibly made without looking at why the change has been suggested.

@pradyunsg
Copy link
Member Author

(for those following via email, I've made quite a few edits to the previous comment)

@gvanrossum
Copy link
Member

I'm glad that you want to help, and I'm sorry I made you feel that way. Let me explain what I meant (though I said it clumsily in an attempt at humor).

I don't recall if the official policy is written up anywhere, and different core developers seem to feel differently about this, but I believe that cleanup PRs are usually not worth it. They add noise to git log and git blame output and take valuable developer time reviewing. This also applies (for me) to spelling/typo fixes and even English grammar fixes, except in prominent places (e.g. fixing a typo in a PEP title is fine) or during the initial development of a PEP. (I apply the same rules to code fixes BTW.)

There's one thing that I do think is worth fixing -- generate pep 0 without trailing spaces. If you can submit a patch for that I would welcome it.

@gvanrossum gvanrossum closed this May 7, 2019
@pradyunsg
Copy link
Member Author

Thanks for the clarification. :)

I am aware of how you feel about cleanup PRs (and I mostly agree). Since you'd approved and merged a similar PR and additionally asked for automation for detecting this situation, I figured this would be an okay change to make.

I personally don't think having extra .rstrip() calls in genpep0 would do any good on its own so I won't be submitting a patch for that.

None the less, it might be handy to close #988 if you think this isn't a change worth making.

@pradyunsg pradyunsg deleted the trailing-whitespace branch December 12, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have pep2html.py detect trailing whitespace
3 participants