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

doc: use ASCII apostrophes consistently #43114

Merged

Conversation

tniessen
Copy link
Member

We can use non-ASCII apostrophes, but we should be consistent. Because the vast majority of apostrophes in our docs is ASCII, this changes the few non-ASCII apostrophes () to ASCII apostrophes ('). This resolves such minor inconsistencies:

example inconsistency using both ASCII and non-ASCII apostrophes

@tniessen tniessen added the doc Issues and PRs related to the documentations. label May 15, 2022
@tniessen tniessen requested a review from Trott May 15, 2022 14:52
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 15, 2022
Copy link
Member Author

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I am going to blindly trust the linter here.

src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented May 15, 2022

This is the wrong direction; straight quotes are never typographically correct in prose, and only curly quotes should be used.

@tniessen
Copy link
Member Author

@ljharb English isn't my first language so I'll gladly defer that decision to someone else. This change is in line with the Wikipedia style guide.

These are very few occurrences. If we do want to go the other way, we have to change thousands of apostrophes.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

I'm fine with either of these provided we use one consistently throughout the docs. Any chance we can enforce the usage of ' with a linter rule?

@Trott
Copy link
Member

Trott commented May 15, 2022

straight quotes are never typographically correct in prose,

That is incorrect. For an overview of the nuance and ambiguity, see https://graphicdesign.stackexchange.com/a/66812

only curly quotes should be used.

If we had sufficient oversight of the markdown documents that we could ensure consistent and correct usage, that might make sense. But we don't. And, as evidenced by the lint issues that were uncovered with this change, our tooling doesn't account for these characters. Using ASCII punctuation helps with grep and other use cases (including our tooling).

@Trott
Copy link
Member

Trott commented May 15, 2022

It looks like the Microsoft Style Guide (which is what we follow in the Node.js docs etc.) uses straight apostrophes although they don't specifically call that out in their text as far as I can tell. https://docs.microsoft.com/en-us/style-guide/punctuation/apostrophes

image

In the headers, the apostrophe character is rendered at an angle, but it is still the ASCII apostrophe and not a non-ASCII Unicode character.

@JakobJingleheimer
Copy link
Member

I don't particularly have an opinion except that if there is no lint rule, this PR will surely become undone very quickly (so why bother).

@Trott
Copy link
Member

Trott commented May 15, 2022

Any chance we can enforce the usage of ' with a linter rule?

Yes. We'd likely need to identify all the possible non-ASCII apostrophe and "smart" quote characters, and then create a remark-lint rule to find them and flag them. Maybe open an issue (or even a pull request) at https://github.com/nodejs/remark-preset-lint-node?

@tniessen
Copy link
Member Author

This is the wrong direction; straight quotes are never typographically correct in prose, and only curly quotes should be used.

I don't know about "typographically correct". The Google style guide, the Microsoft style guide, and the Wikipedia style guide all use straight apostrophes and the latter explicitly recommends not to use curly apostrophes. I've found some other style guides that use straight apostrophes for possessives but curly quotation marks for actual quotes (e.g., the SUSE Documentation Style Guide).

Enforcing curly apostrophes would probably be difficult because straight apostrophes are used in code, but this direction should be relatively simple to enforce.

@GeoffreyBooth
Copy link
Member

I feel pretty strongly that curly quotes are the correct style for humans to read. Perhaps the original files can have straight quotes and we generate curly ones as part of generating the HTML?

@tniessen
Copy link
Member Author

I feel pretty strongly that curly quotes are the correct style for humans to read. Perhaps the original files can have straight quotes and we generate curly ones as part of generating the HTML?

That seems reasonable to me if we have a method that produces consistent and correct output, and a good justification for the added complexity in doc/README.md.

@Trott
Copy link
Member

Trott commented May 15, 2022

I feel pretty strongly that curly quotes are the correct style for humans to read. Perhaps the original files can have straight quotes and we generate curly ones as part of generating the HTML?

I wouldn't want to literally change the character from ASCII 39 to some other code point, but might we consider choosing a font for text that renders ASCII 39 curved while continue using a font for code that renders it straight? I'm not even sure if that's a thing, though.

@aduh95
Copy link
Contributor

aduh95 commented May 15, 2022

I feel pretty strongly that using the ASCII quote is the "correct" choice for our markdown documents, because that's the one that's on most keyboard layouts. Sure that's something that could be addressed by making make format-md silently convert to curly quotes, but it would still be a pain when e.g. making suggestions on a PR review.

I wouldn't want to literally change the character from ASCII 39 to some other code point, but might we consider choosing a font for text that renders ASCII 39 curved while continue using a font for code that renders it straight? I'm not even sure if that's a thing, though.

That sounds reasonable, if that's "not a thing" we could fork an existing open source font and make it happen if folks feel strongly about it.

@tniessen tniessen force-pushed the doc-consistent-apostrophes branch from e21761c to 736fa09 Compare May 15, 2022 19:15
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2022
@GeoffreyBooth
Copy link
Member

That sounds reasonable, if that’s “not a thing” we could fork an existing open source font and make it happen if folks feel strongly about it.

The way I’ve usually seen this implemented is that plugins do this during HTML generation. Then you can use any font you want.

From a cursory glance at our docs generation scripts, it appears that we use https://github.com/remarkjs/remark to parse our Markdown, with a bunch of plugins added. There’s a Remark plugin https://github.com/silvenon/remark-smartypants which adds the SmartyPants typography, which is smart/curly quotes plus hyphens and ellipses. Perhaps we could investigate an approach like this?

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit 895cc57 into nodejs:master May 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 895cc57

bengl pushed a commit that referenced this pull request May 30, 2022
PR-URL: #43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@bengl bengl mentioned this pull request May 31, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43114
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants