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: typography mistake #45398

Merged
merged 1 commit into from
Nov 13, 2022
Merged

doc: typography mistake #45398

merged 1 commit into from
Nov 13, 2022

Conversation

kovsu
Copy link
Contributor

@kovsu kovsu commented Nov 10, 2022

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Nov 10, 2022
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

This is not a spelling mistake although it is awkward typography.

@Trott
Copy link
Member

Trott commented Nov 10, 2022

The backticks do not render as punctuation. They are there for formatting/typography.

image

It should be reworded though, because using the keyword as a verb is weird. Instead of "required" maybe we can say:

imported via `require()`

...or:

included via `require()`

...or something like that.

@kovsu
Copy link
Contributor Author

kovsu commented Nov 10, 2022

This is not a spelling mistake although it is awkward typography.

I don't know how to express it 😀. And thanks for immediately correcting my mistake.

@kovsu kovsu changed the title doc: spelling mistake doc: typography mistake Nov 10, 2022
@kovsu kovsu requested a review from Trott November 10, 2022 11:00
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Do we have an official way of styling this? For example, (outside of nodejs docs) I write

require()ed
imported
import()ed

@Trott
Copy link
Member

Trott commented Nov 11, 2022

Do we have an official way of styling this? For example, (outside of nodejs docs) I write

require()ed
imported
import()ed

I've been looking for guidance in the Microsoft Style Guide (which we try to follow) but haven't found any. My personal opinion, though, is that we should avoid using function names as verbs.

  • Both the formatted typography and the bare markdown can be confusing when we do that. That's the whole reason this PR was opened. The reader thought there was a mistake when reading the markdown.
  • Not all function names use words that are verbs. Ideally we should adopt a practice that can be applied consistently, so that means not using function names as verbs because a string is not "indexOf()ed".
  • It can be unclear if the function arguments are the verb's subject ("the array is map()ed") or an object ("the module is import()ed").
  • Using backticks in the middle of a word in markdown usually results in jarringly out-of-proportion spacing in the resulting formatted text. You can see it in the bullet point above where it looks like "import() ed" instead of "import()ed".

For these reasons (and probably a few other reasons I haven't thought of), I think it's best to use a separate verb and let the function name be what it actually is, which is a noun. It might be good to standardize for some specific cases, though. This PR uses "included via require()" but it could have been "loaded" instead of "included" and probably a bunch of others options.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 11, 2022
@JakobJingleheimer
Copy link
Member

I think it would be good to establish some kind of policy, otherwise we'll end up with a bunch of these issues/PRs.

Colloquial English turns pretty much any word into a verb/past-participle by just adding "ed" to the end, so I think it's acceptable here.

RE confusion about to what it's referring, that's why I include the parentheses: require()ed. Then it's pretty clear it's a function whose name is require. It's a little less clear when it's not a function, like awaited and imported. I don't find the extra space difficult to figure out, but perhaps because I'm familiar with the convention and reading formatted markdown.

But I'm also totally fine with "included via require()" (with parens) and friends.

@kovsu kovsu closed this Nov 13, 2022
@Trott Trott reopened this Nov 13, 2022
@Trott Trott added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 13, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-bot nodejs-github-bot merged commit c4d75ea into nodejs:main Nov 13, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in c4d75ea

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
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. doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants