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

doc: replace bash references with sh by zackschuster · Pull Request #34869 · nodejs/node · GitHub

/ node Public

doc: replace bash references with sh - #34869

Closed
zackschuster wants to merge 1 commit into
nodejs:masterfrom
zackschuster:docs-bash-to-sh
Closed

doc: replace bash references with sh#34869
zackschuster wants to merge 1 commit into
nodejs:masterfrom
zackschuster:docs-bash-to-sh

Conversation

zackschuster commented Aug 21, 2020
edited
Loading

Copy link
Copy Markdown

sh means the POSIX interface, and could be bash, zsh, etc. — even just plain old sh.

refs: #34837

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

zackschuster requested review from a team as code owners August 21, 2020 22:28
nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. npm Issues and PRs related to the npm client dependency or the npm registry. labels Aug 21, 2020

Copy link
Copy Markdown
Author

@DerekNonGeneric @Trott

Comment thread deps/node-inspect/README.md Outdated

zackschuster commented Aug 21, 2020
edited
Loading

Copy link
Copy Markdown
Author

not sure why lint-md is failing. according to tools/lint-md.js#L47139 it should be allowed.

DerekNonGeneric commented Aug 21, 2020
edited
Loading

Copy link
Copy Markdown
Contributor

We chose bashon purpose. I thought you'd specify further. Are you sure this isn't csh?

Comment thread doc/api/fs.md Outdated

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
Suggested change
```sh
```console

Copy link
Copy Markdown
Author

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 understand this request. isn't console for js code?

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

console means a CLI, it usually shows a $, then a command, and below the standard output it produces. I don't think it's ever used for JS code – you would rather use javascript or js for JS code.

Copy link
Copy Markdown
Author

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

ah, i see. i updated the code. thanks for pointing that out @DerekNonGeneric 😄

zackschuster commented Aug 21, 2020
edited
Loading

Copy link
Copy Markdown
Author

Are you sure this isn't csh?

@DerekNonGeneric i've never used csh; i only mention it because it was mentioned in the other PR.

We chose bash on purpose. I thought you'd specify further.

what would you like to see elaborated?

Copy link
Copy Markdown
Contributor

It's not sh. That's too obscure. Find another info string.

Copy link
Copy Markdown
Author

as was pointed out in the other PR, the syntax highlighting remains the same. using implementation-agnostic references is more correct, both technically (as node itself is officially POSIX compliant) and semantically (as sh is inclusive to both itself and bash et al.)

Copy link
Copy Markdown
Contributor

Yes, but we don't want to make mistakes or repeat this over and over.

Copy link
Copy Markdown
Author

of course :) i amended OP to fix the mistake. thanks for pointing it out.

aduh95 commented Aug 22, 2020

Copy link
Copy Markdown
Contributor

IMHO sh is the correct infostring to use, another one could be shell. bash is just one specific shell, that happens to be the default on some Linux distribution, but there is no reason to use it in our docs. Thanks for doing that @zackschuster :)

Copy link
Copy Markdown
Contributor

The problem here is that we don't want other contributors to make this mistake and it will cause errors later as well. Should we change it? What do we do? I'm just as at a loss here. This has a lot to do with having made choices already. @aduh95, I think you know what is going on more than I do, but things are getting difficult to follow.

Copy link
Copy Markdown
Author

i'm a bit unclear myself — what is the exact issue? from what i gather it seems to be "a csh user might think the sh references include them". if that's the case, i would argue the inverse is true as well; as things stand, a zsh user (say, a new developer on a fresh catalina machine) might assume they're being excluded by the bash references.

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Author

@DerekNonGeneric i was going to mention the PR in the other repo, but it seems you've already commented there.

ruyadorno removed the npm Issues and PRs related to the npm client dependency or the npm registry. label Sep 1, 2020

aduh95 commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

@zackschuster do you still want to do that? Asking because I still have the PR open on the other repo, and I was wondering if I should close it or request a merge.

aduh95 added the stalled Issues and PRs that are stalled. label Oct 12, 2020

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Copy link
Copy Markdown
Author

@aduh95 yes please. i'll rebase once the other PR is merged 😄

aduh95 commented May 29, 2021

Copy link
Copy Markdown
Contributor

Closing, as nodejs/remark-preset-lint-node#136 didn't get merged.

aduh95 closed this May 29, 2021
zackschuster deleted the docs-bash-to-sh branch May 31, 2021 06:11
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL