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

Replace trailing space line breaks with backslash line breaks and remove all other trailing whitespace #10470

Merged
merged 9 commits into from
Nov 27, 2023

Conversation

Swekka
Copy link
Contributor

@Swekka Swekka commented Nov 15, 2023

Steps to recreate

If you run remark on 1a574a9 you will find there are 1372 'Hard breaks should be made with a backslash' warnings.
If you search for the regex {2,}\n you will find 1468 matches
This means there are 1468-1372=96 instances of trailing whitespace that doesn't do anything.
The first two commits only do the regexes in news/ because wiki/ doesn't have any 'Hard breaks should be made with a backslash' warnings.

First commit (news/ only) Remove all instances of trailing whitespace that doesn't do anything

  1. Remove trailing spaces followed by blank lines:
find news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/ {2,}\n( *\n)/\n\1/g' {} +

(replaces 71 instances in 44 files)
2. Remove trailing spaces after list items:
Run this twice

find news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/\n-([^\n]*) {2,}\n/\n-\1\n/g' {} +

(replaces 10 instances in 8 files)
3. Remove spaces on blank lines that consist entirely of spaces:

find news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/\n {2,}\n/\n\n/g' {} +

(replaces 14 instances in 11 files)
4. Remove trailing spaces in HTML: Replace (>) {2,}+\n with $1\n

find news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/> {2,}+\n/>\n/g' {} +

(replaces 1 instance)
71+10+14+1=96 meaning all instances of trailing whitespace that doesn't do anything has been removed.
If you re-run remark after this commit the amount of total warnings and 'Hard breaks should be made with a backslash' warnings will be the same, indicating that the trailing spaces I removed weren't line breaks.

Second commit (news/ only) Replace trailing space line breaks with backslash line breaks

find news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/ {2,}\n/\\\n/g' {} +

If you re-run remark after this commit all 1372 'Hard breaks should be made with a backslash' warnings will be gone

Third commit (news/ and wiki/) Remove all remaining trailing whitespace

Since all 'Hard breaks should be made with a backslash' warnings are gone, we know the remaining whitespace doesn't do anything so it can be removed.

find wiki news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/ +(\n|$)/\1/g' {} +

Fourth commit (news/ and wiki/) Replace consecutive blank lines with single blank line (addresses the new CI failures)

find wiki news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/\n{3,}/\n\n/g' {} +

Self-check

SKIP_WIKILINK_CHECK SKIP_OUTDATED_CHECK

@Swekka Swekka requested review from peppy, Ephemeralis and a team as code owners November 15, 2023 09:53

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@peppy
Copy link
Member

peppy commented Nov 15, 2023

Probably need someone to convert to sed commands.

@Swekka
Copy link
Contributor Author

Swekka commented Nov 15, 2023

I converted them

This comment was marked as resolved.

@peppy
Copy link
Member

peppy commented Nov 16, 2023

You're going to need to check CI as there are new failures.

@Swekka
Copy link
Contributor Author

Swekka commented Nov 16, 2023

The new CI failures should be fixed now.

@peppy
Copy link
Member

peppy commented Nov 16, 2023

nope

cl8n
cl8n previously approved these changes Nov 22, 2023
Copy link
Member

@cl8n cl8n left a comment

Choose a reason for hiding this comment

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

strategy for the automated edits looks good & i manually checked the first few years of news posts since those are always a nightmare...

@cl8n
Copy link
Member

cl8n commented Nov 22, 2023

@ DoryanR These remark warnings and other wrongly formatted things were already there before my PR. I plan on making more PR's to fix most of these but it's out of scope for this PR. Peppy was talking about new remark warnings that this PR introduced but I've fixed all of them now.

if you do more remark things please try to use remark's output (-o) to solve the issue, and fix the wiki articles or remark itself as necessary, the eventual goal is that remark will print something we're happy with and we don't have to do these kinds of PRs anymore

@Swekka
Copy link
Contributor Author

Swekka commented Nov 22, 2023

I think it's a lot more efficient to do things rule by rule. e.g. making one PR for removing trailing whitespace, one PR for using the right style for emphasis than to make a PR for whitespace but also fix all remark warnings in all edited files when I'm planning to make seperate PR's for those remark rules anyways.

To put it more concretely:

  1. PR for removing all trailing whitespace
  2. PR for fixing all emphasis style

is more efficient than

  1. PR for removing all trailing whitespace
  2. Fixing emphasis style in that PR
  3. PR for fixing all emphasis style

@cl8n
Copy link
Member

cl8n commented Nov 22, 2023

I'm not saying to include unrelated changes in same PR. what I mean is that remark can print markdown itself using its -o flag, and we should be improving that system at higher priority than large find+replace types of PRs like this one. theoretically this whole class of changes would be irrelevant if we could confidently run remark -o over the whole wiki.

this PR still helps on its own, I just want to make sure we're looking at remark -o along the way for these types of changes and verifying that it is correct as well

related but also very outdated: #3310 #2275

This comment was marked as resolved.

TicClick
TicClick previously approved these changes Nov 23, 2023
Copy link
Contributor

@TicClick TicClick left a comment

Choose a reason for hiding this comment

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

lgtm for everything from wiki

Copy link
Member

@Walavouchey Walavouchey left a comment

Choose a reason for hiding this comment

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

i've skimmed the backslash changes and looks good otherwise for the news part

news/2016/2016-08-26-are-huge-sets-a-problem.md Outdated Show resolved Hide resolved
@Swekka Swekka dismissed stale reviews from TicClick and cl8n via d5bdf78 November 23, 2023 17:35
@Walavouchey
Copy link
Member

Walavouchey commented Nov 23, 2023

i've verified locally that the provided command (find wiki news -name "*.md" -type f -exec sed -i -E ':a; N; $!ba; s/ +(\n|$)/\1/g' {} +) produces a matching diffstat for c43d432, the largest commit (having skimmed the other commits' diffs)

@Walavouchey
Copy link
Member

@peppy @Ephemeralis this touches help centre and will thus need a tap from either of you

Copy link

Your translation may be missing new pending changes to English articles. Please update your translation after they are merged:

@peppy peppy disabled auto-merge November 27, 2023 01:32
@peppy peppy merged commit 39c6a74 into ppy:master Nov 27, 2023
2 checks passed
@Swekka Swekka deleted the remark-trailing-whitespace2 branch November 27, 2023 10:50
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.

6 participants