FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix typo in --specifier-resolution=[node] by Urigo · Pull Request #39249 · nodejs/node · GitHub

/ node Public

Fix typo in --specifier-resolution=[node] - #39249

Closed
Urigo wants to merge 3 commits into
nodejs:mainfrom
Urigo:patch-1
ConversationCommits (3)ChecksFiles changed
Closed

Fix typo in --specifier-resolution=[node]#39249
Urigo wants to merge 3 commits into
nodejs:mainfrom
Urigo:patch-1

Conversation

Urigo commented Jul 3, 2021

Copy link
Copy Markdown

No description provided.

github-actions Bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jul 3, 2021
mscdex reviewed Jul 3, 2021
View reviewed changes
Comment thread doc/api/esm.md Outdated
file.

The `--experimental-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[node]` flag can be used to customize

mscdex Jul 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't believe this change is correct. "mode" here is referring to the parameter name, whereas "node" is one possible value for the "mode" parameter. The "mode" could also have a value of "explicit".

benjamingr requested changes Jul 3, 2021
View reviewed changes

benjamingr left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for this PR like mscdex said this is not a typo :)

Changes to the docs making that clearer are welcome though I'm personally not sure how to make it clearer.

Urigo commented Jul 3, 2021

Copy link
Copy Markdown
Author

ohh got it, sorry about that :)

mmm I guess maybe something different then mode? maybe resolution-mode ?

gireeshpunathil commented Jul 4, 2021

Copy link
Copy Markdown
Member

mmm I guess maybe something different then mode? maybe resolution-mode ?

from the very fact that this PR exists shows that the word mode is confusing, and makes sense to change it, though resolution-mode looks a bit longer for the purpose?

/cc @nodejs/loaders

benjamingr commented Jul 4, 2021

Copy link
Copy Markdown
Member

I think resolution-mode is perfectly fine (especially since it's only for the docs example and not the actual flag) :)

PR update welcome

Urigo commented Jul 4, 2021

Copy link
Copy Markdown
Author

Thanks for the comments!
Updated

tniessen reviewed Jul 5, 2021
View reviewed changes
Comment thread doc/api/esm.md Outdated
file.

The `--experimental-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[resolution-mode]` flag can be used to customize

tniessen Jul 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This line now exceeds 80 characters, while the rest of the paragraph is wrapped at 80 chars. Not sure whether we usually fix this.

JakobJingleheimer Jul 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, it fails the lint doc check, which blocks merging.

benjamingr Jul 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@tniessen in my anecdotal experience we usually fix this for first time contributors if they are unable to do so themselves.

benjamingr approved these changes Jul 6, 2021
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fix looks good - please drop it to two lines in order to meet the 80 character limit :)

aduh95 commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

The flag was removed in #44859.

aduh95 closed this Apr 22, 2023

benjamingr commented Apr 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Sorry for dropping the ball on getting this through the finish line back then @Urigo ! Thank you for your contribution

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
JakobJingleheimer JakobJingleheimer left review comments
tniessen tniessen left review comments
gireeshpunathil gireeshpunathil approved these changes
aduh95 aduh95 approved these changes
HarshithaKP HarshithaKP approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL