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

Lint: Normalise whitespace #4078

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Oct 22, 2024

cc @lysnikolaou (illustation of #4067)


📚 Documentation preview 📚: https://pep-previews--4078.org.readthedocs.build/

@lysnikolaou
Copy link
Member

Yup, this is probably okay!

@hugovk
Copy link
Member

hugovk commented Oct 22, 2024

I'm less keen on the more explicit lists: realistically, we're unlikely to fill in any of the gaps for older numbers.

I'm leaning more towards just applying the fixes to all files in one fell swoop.

@lysnikolaou
Copy link
Member

I'm leaning more towards just applying the fixes to all files in one fell swoop.

Same. I say we bite the bullet and just do it everywhere.

@willingc
Copy link
Contributor

@lysnikolaou YOLO

@AA-Turner AA-Turner added the lint Linter-related work and linting fixes on PEPs label Oct 24, 2024
name: "Ensure files end with a single newline"
- id: trailing-whitespace
name: "Remove trailing whitespace"
args: ["--chars", " \t"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only trim trailing spaces and tabs? What trailing whitespace do we want to conserve?

https://github.com/pre-commit/pre-commit-hooks?tab=readme-ov-file#trailing-whitespace

Copy link
Member Author

@AA-Turner AA-Turner Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Form-feed characters before the emacs stanza in older PEPs. I'd want to remove the stanzas and the form-feed characters at the same time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we do it in this PR? How many extra files and pings would it be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~290 files (searching for " Local Variables:") Currently this PR touches 90 files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And 37 -> 81 pings...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking with #4081 was to include 'everything' in one PR, so we don't do a mass ping again (or, try and minimise the number of mass pings). It's a fine line, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint Linter-related work and linting fixes on PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants