GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: typography mistake by kovsu · Pull Request #45398 · nodejs/node

Conversation

@kovsu

@kovsu kovsu commented Nov 10, 2022

Copy link
Copy Markdown
Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. module Issues and PRs related to the module subsystem. labels Nov 10, 2022
Trott
Trott requested changes Nov 10, 2022
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kovsu and aduh95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@Trott

Trott commented Nov 10, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kovsu

kovsu commented Nov 10, 2022

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
Trott
Trott reviewed Nov 11, 2022
View reviewed changes
Comment thread doc/api/modules.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Nov 11, 2022
View reviewed changes
Comment thread doc/api/modules.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Nov 11, 2022
View reviewed changes
Comment thread doc/api/modules.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Nov 11, 2022
View reviewed changes
Comment thread doc/api/modules.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Nov 11, 2022
View reviewed changes
Comment thread doc/api/modules.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@kovsu @Trott
doc: adjust wording to eliminate awkward typography
379e1c0
Trott
Trott approved these changes Nov 11, 2022
View reviewed changes
JakobJingleheimer
JakobJingleheimer approved these changes Nov 11, 2022
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Trott

Trott commented Nov 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott Trott added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 11, 2022
@JakobJingleheimer

JakobJingleheimer commented Nov 11, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@kovsu kovsu closed this Nov 13, 2022
@Trott Trott reopened this Nov 13, 2022
@Trott Trott added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 13, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 13, 2022
@nodejs-github-bot
nodejs-github-bot merged commit c4d75ea into nodejs:main Nov 13, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2022

Copy link
Copy Markdown
Collaborator

Landed in c4d75ea

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@kovsu @ruyadorno
doc: adjust wording to eliminate awkward typography
94a6a97
PR-URL: #45398
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
v19.2.0 proposal #45615
Merged
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@kovsu @danielleadams
doc: adjust wording to eliminate awkward typography
6ebab65
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
@kovsu @danielleadams
doc: adjust wording to eliminate awkward typography
c6b89ab
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

Reviewers

@Trott Trott Trott approved these changes
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. module Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@kovsu @nodejs-github-bot @Trott @JakobJingleheimer
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.