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 'console' info string for console output #34837

Closed
wants to merge 0 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 19, 2020

We don't want bash syntax highlighting for command-line examples, so
switch to text or, where appropriate, console.

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 19, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

doc/api/esm.md Outdated
@@ -1895,7 +1885,7 @@ requires the full path to a module be provided to the loader. To enable the
automatic extension resolution and importing from directories that include an
index file use the `node` mode.

```bash
```console
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only change that should be made. I've seen this and known about it.

doc/api/esm.md Outdated
Comment on lines 97 to 98
If in the same folder as the above package.json, `node my-app.js` runs
`my-app.js` as an ES module.
Copy link
Member

Choose a reason for hiding this comment

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

in this example, the command no longer stands out with this change, and I find it less readable. why don't we want syntax highlighting on command line commands?

@DerekNonGeneric
Copy link
Contributor

We don't want bash syntax highlighting for command-line examples […]

That's actually untrue. We do want bash syntax for all non-console command lines.

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Aug 19, 2020

Here's the reasoning: each single command line isn't shell-agnostic, which is what console was being used for in the past. There is no such thing as a shell-agnostic command line (unless it starts w/ node [or others and doesn't use &&, |, et al. after it], then it's up to that command's command-line argument parser to do the work), so we've chosen to go w/ Bash due to its POSIXness. I personally am not a Bash user (for my daily routine), but keeping the docs in Bash is perfectly fine. Anything else would be a mistake IMO.

@Trott
Copy link
Member Author

Trott commented Aug 20, 2020

We don't want bash syntax highlighting for command-line examples […]

That's actually untrue. We do want bash syntax for all non-console command lines.

My thinking was:

  • We don't know that it's a bash shell. Not just csh or whatever, but a lot of these could be Windows commands.
  • Seeing syntax highlighting on the command line may be confusing.

But really those are rationales probably, and I think mostly I've just seen too much misleading bash highlighting when the command prompt and output is included.

So happy to revise this to just be the one uncontroversial instance. PTAL.

@DerekNonGeneric
Copy link
Contributor

a lot of these could be Windows commands

It's impossible to use git on Windows w/o first installing Git Bash.

Any dev using Windows would know this. I think we should think about the target audience here.

@DerekNonGeneric
Copy link
Contributor

Well, let me correct myself, because that's not entirely true. There are other ways to do use git on Windows, but changing this to anything other than bash is more of a philosophical discussion and is way off-topic. lol

@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

Well, let me correct myself, because that's not entirely true. There are other ways to do use git on Windows, but changing this to anything other than bash is more of a philosophical discussion and is way off-topic. lol

I certainly agree that the only logical choices (as far as I can tell) are bash or text and I'm happy to leave it as bash. My preference for text is very mild. (I don't think command lines need syntax highlighting, usually. Maybe when they get complex, but that generally should be avoided in documentation. There are other reasonable perspectives on this, though, and I'm happy to just go with bash.)

@zackschuster
Copy link

would sh accomplish the same thing? that could be a more agnostic choice, i think.

@DerekNonGeneric
Copy link
Contributor

would sh accomplish the same thing? that could be a more agnostic choice, i think.

There is a point to choosing bash, which is stated above.

There is no such thing as a shell-agnostic command line

@zackschuster, the question I would have to ask is what does sh mean to you?

@zackschuster
Copy link

what does sh mean to you?

@DerekNonGeneric sh means the POSIX interface, and could be bash, zsh, csh or even just plain old sh. the argument is that representing an interface makes it a "more agnostic" reference than any particular implementation.

@zackschuster
Copy link

zackschuster commented Aug 21, 2020

(this assumes sh and bash are otherwise equal wrt syntax highlighting, etc. of course 😄)

@DerekNonGeneric
Copy link
Contributor

@zackschuster, everything here is in Bash, what can I do? lol

@zackschuster
Copy link

i can open a PR myself 😄

@DerekNonGeneric
Copy link
Contributor

i can open a PR myself 😄

Would you?

@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

would sh accomplish the same thing? that could be a more agnostic choice, i think.

It may be semantically more agnostic, but under the hood, I believe it is identical to bash. So the rendering will be the same.

(I'd still be in favor of such a change for semantic reasons.)

@Trott Trott added the review wanted PRs that need reviews. label Aug 21, 2020
@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

This very small change needs reviews. Other than that, it is ready to land.

@Trott Trott changed the title doc: remove 'bash' info string for things that aren't bash files doc: use 'console' info string for console output Aug 21, 2020
Trott added a commit that referenced this pull request Aug 21, 2020
PR-URL: #34837
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

Landed in fc6f136

@Trott Trott closed this Aug 21, 2020
@Trott Trott deleted the bash branch August 21, 2020 13:18
BethGriggs pushed a commit that referenced this pull request Aug 24, 2020
PR-URL: #34837
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@danielleadams danielleadams mentioned this pull request Aug 25, 2020
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34837
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34837
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants