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: replace bash references with sh #34869

Closed
wants to merge 1 commit into from
Closed

doc: replace bash references with sh #34869

wants to merge 1 commit into from

Conversation

zackschuster
Copy link

@zackschuster zackschuster commented Aug 21, 2020

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 zackschuster requested review from a team as code owners August 21, 2020 22:28
@nodejs-github-bot 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
@zackschuster
Copy link
Author

@DerekNonGeneric @Trott

deps/node-inspect/README.md Outdated Show resolved Hide resolved
@zackschuster
Copy link
Author

zackschuster commented Aug 21, 2020

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

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Aug 21, 2020

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

doc/api/fs.md Outdated
@@ -3732,7 +3732,7 @@ fs.symlink('./mew', './example/mewtwo', callback);
The above example creates a symbolic link `mewtwo` in the `example` which points
to `mew` in the same directory:

```bash
```sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```sh
```console

Copy link
Author

Choose a reason for hiding this comment

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

i don't understand this request. isn't console for js code?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Author

Choose a reason for hiding this comment

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

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

@zackschuster
Copy link
Author

zackschuster commented Aug 21, 2020

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?

@DerekNonGeneric
Copy link
Contributor

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

@zackschuster
Copy link
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.)

@DerekNonGeneric
Copy link
Contributor

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

@zackschuster
Copy link
Author

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

@aduh95
Copy link
Contributor

aduh95 commented Aug 22, 2020

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 :)

@DerekNonGeneric
Copy link
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.

@zackschuster
Copy link
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.

@DerekNonGeneric
Copy link
Contributor

@zackschuster
Copy link
Author

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

@ruyadorno ruyadorno removed the npm Issues and PRs related to the npm client dependency or the npm registry. label Sep 1, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2020

@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 aduh95 added the stalled Issues and PRs that are stalled. label Oct 12, 2020
@github-actions
Copy link
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.

@zackschuster
Copy link
Author

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

@aduh95
Copy link
Contributor

aduh95 commented May 29, 2021

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

@aduh95 aduh95 closed this May 29, 2021
@zackschuster zackschuster deleted the docs-bash-to-sh branch May 31, 2021 06:11
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