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.

Sign up for GitHub

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

Jump to bottom

[v12.x backport] doc: remove em dashes #32146

Closed
wants to merge 2 commits into from
Closed

[v12.x backport] doc: remove em dashes #32146

wants to merge 2 commits into from

Conversation

puzpuzpuz
Copy link
Member

Backports the following PRs to 12.x-staging:

Both of these PRs are necessary to have a successful build on 12.x-staging. See #32131 (comment) and #32131 (comment) to understand the context.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@sam-github @puzpuzpuz
test: mark empty udp tests flaky on OS X
0b5fda0 
They fail on OS X 10.15 (aka "Catalina"), but pass on earlier OS X.

Refs: nodejs#30030
Refs: nodejs/build#2189 (comment)

PR-URL: nodejs#31936
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. v12.x labels Mar 8, 2020
This was referenced Mar 8, 2020
Closed
Merged
@Trott @puzpuzpuz
doc: remove em dashes
f5a4000 
Our documentation uses em dashes inconsistently. They are treated
inconsistently typographically too. (For example, they are sometimes
surrounded by spaces and sometimes not.) They are also often confused
with ordinary hyphens such as in the CHANGELOG, where they are
inadvertently mixed together in a single list. The difference is
not obvious in the raw markdown but is very noticeable when rendered,
appearing to be a typographical error (which it in fact is).

The em dash is never needed. There are always alternatives. Remove em
dashes entirely.

PR-URL: nodejs#32080
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@puzpuzpuz
Copy link
Member Author

puzpuzpuz commented Mar 8, 2020 •
edited

It looks like Travis CI failed due to timeout in compilation phase. I've already seen such failure here: #32131 (comment)

Both compilation and tests run successfully on my machine.

@puzpuzpuz puzpuzpuz changed the title [v12.x] doc: backport remove em dashes to v12.x [v12.x backport] doc: remove em dashes Mar 8, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@puzpuzpuz
Copy link
Member Author

puzpuzpuz commented Mar 8, 2020 •
edited

CI: https://ci.nodejs.org/job/node-test-pull-request/29631/

It seems that there are some flaky tests remaining in v12.x-staging:

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29637/

@puzpuzpuz
Copy link
Member Author

@vdeturckheim @Qard guys, could one of you review this one? We need it to unblock #32131

vdeturckheim
vdeturckheim approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

LGTM

@puzpuzpuz
Copy link
Member Author

Is there something I should do here or this PR is ready to land? I have no experience with backports in core, so that's why I'm asking.

@Trott
Copy link
Member

Trott commented Mar 10, 2020

Is there something I should do here or this PR is ready to land? I have no experience with backports in core, so that's why I'm asking.

Once a 12.x release is being prepared, someone on @nodejs/releasers will start landing these. I don't think there's anything to do here other than wait. Since 12.x is an LTS release, only releasers are supposed to land stuff to the 12.x-staging branch.

@puzpuzpuz
Copy link
Member Author

@Trott thanks for the clarification. Then I'll be waiting for a releaser on this one. Once this one is merged, I'll be unblocked with #32131 and that PR will be able to have a successful CI build.

@richardlau richardlau mentioned this pull request Mar 10, 2020
Closed
@codebytere
Copy link
Member

Landed in 5a537ba..ebc47f8

@codebytere codebytere closed this Mar 14, 2020
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@sam-github @codebytere
test: mark empty udp tests flaky on OS X
5a537ba 
They fail on OS X 10.15 (aka "Catalina"), but pass on earlier OS X.

Refs: #30030
Refs: nodejs/build#2189 (comment)

PR-URL: #31936
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>

PR-URL: #32146
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@Trott @codebytere
doc: remove em dashes
ebc47f8 
Our documentation uses em dashes inconsistently. They are treated
inconsistently typographically too. (For example, they are sometimes
surrounded by spaces and sometimes not.) They are also often confused
with ordinary hyphens such as in the CHANGELOG, where they are
inadvertently mixed together in a single list. The difference is
not obvious in the raw markdown but is very noticeable when rendered,
appearing to be a typographical error (which it in fact is).

The em dash is never needed. There are always alternatives. Remove em
dashes entirely.

PR-URL: #32080
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #32146
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@puzpuzpuz puzpuzpuz deleted the remove-dash-backport-v12.x branch March 14, 2020 19:46
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

@Trott Trott Trott approved these changes

@codebytere codebytere codebytere approved these changes

@vdeturckheim vdeturckheim vdeturckheim approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@puzpuzpuz @nodejs-github-bot @Trott @codebytere @Qard @vdeturckheim @BridgeAR @sam-github