Details

Reviewers

varun
ashoat

Commits

rCOMM0c97665eb85a: [docs] Fix `nvm` setup instructions

Summary

Fix nvm setup instructions by adding a code snippet Homebrew prints for M1 Macs under the Homebrew instructions portion of the nvm setup.

Related Linear issue with full context here.

The current code snippet on the dev environment setup is

export NVM_DIR="$HOME/.nvm"
[ -s "/usr/local/opt/nvm/nvm.sh" ] && . "/usr/local/opt/nvm/nvm.sh" --no-use # This loads nvm
[ -s "/usr/local/opt/nvm/etc/bash_completion.d/nvm" ] && . "/usr/local/opt/nvm/etc/bash_completion.d/nvm"  # This loads nvm bash_completion

This code snippet is only printed by Homebrew on Intel Macs, the updated snippet on an M1 Mac looks like:

Updated the docs to acknowledge the differing outputs based on the user's processor, would love to hear feedback regarding the exact verbiage and wording.

Test Plan

Ran the dev environment setup on my M1 Macbook and copied the code snippet Homebrew printed at this step.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• abosh created this revision.Apr 27 2022, 4:28 PM

Herald added subscribers: • karol, atul, • adrian, tomek. · View Herald TranscriptApr 27 2022, 4:28 PM

• abosh edited the summary of this revision. (Show Details)Apr 27 2022, 4:31 PM

• abosh added 2 blocking reviewer(s): varun, ashoat.

Harbormaster completed remote builds in B8624: Diff 12014.Apr 27 2022, 4:33 PM

• abosh requested review of this revision.Apr 27 2022, 4:33 PM

• abosh added inline comments.Apr 27 2022, 4:37 PM

docs/dev_environment.md
102	Why is there a `\.` here when there is only a `.` used after the `&&` in the current dev environment setup? This is what Homebrew printed when I went through the dev environment setup. See the image in the diff summary. Did some research on the difference between `\.` and `.` and they are both parsed the same way, but `\.` provides the extra safety of interpreting the `.` literally during parsing. This can be useful in the case when the user has aliased `.` to a different command. For these two reasons, I think the current dev environment snippet for Intel Macs below should also be amended to use `\.` after the `&&` instead of only `.`, but this can be addressed in a later diff.

• abosh edited the summary of this revision. (Show Details)Apr 27 2022, 4:39 PM

When you submit a docs diff, you must consider yourself the editor of the docs. It is your responsibility to maintain consistency of tone and voice, to make sure the language flows cleanly, to make sure that the there is no ambiguity, to make sure that relevant concepts are explained, etc. Please be extra thoughtful about docs diffs going forward!

docs/dev_environment.md
96–98	Both of these lines end up with a colon. That doesn't make sense to me...
98	The order of the language here is confusing. You claim that Homebrew will print these lines, but that's not true... the `--no-use` is something we added. Before your edits, the narrative was clear... Homebrew prints something, and then we recommend you append something and here's what it looks like. After your edits, there is an inaccurate claim of what Homebrew prints, and it's not clear that we are applying edits to that.
102	Great work documenting this!! I think you can just make this change in the same diff honestly, but separate diff works too

This revision now requires changes to proceed.Apr 27 2022, 6:14 PM

Address feedback about inaccurate claim with what Homebrew prints and order of narrative. Also replaced . with \. as per inline comment.

Removed extraneous colon (was in Markdown editor, just forgot to include it in VS Code in last commit)

Harbormaster completed remote builds in B8630: Diff 12020.Apr 27 2022, 6:58 PM

Harbormaster completed remote builds in B8631: Diff 12021.Apr 27 2022, 7:02 PM

ashoat requested changes to this revision.Apr 27 2022, 7:13 PM

ashoat added inline comments.

docs/dev_environment.md
96–112 ↗	(On Diff #12021)	For apostrophes, please replace your use of the single quote `'` character with an apostrophe `’` character
98 ↗	(On Diff #12021)	We don't seem to use the pair of terms "Apple Silicon Mac" or "Intel Mac" anywhere else in the docs. Again: It is your responsibility to maintain consistency Please always try to look for precedents / examples in existing docs language when drafting new language.

This revision now requires changes to proceed.Apr 27 2022, 7:13 PM

Address feedback about Apple Silicon/Intel Mac wording. Looked through the docs and saw that the S in Apple silicon was lowercase (as it should be) and Intel is referred to as Intel x86-64. The term "Apple silicon Macs" is only referred to in the Python 2 section, but not paired with "Intel x86-64 Macs," so this has been revised. Also replaced all single quote characters with apostrophes.

Okay, looks good!

Harbormaster completed remote builds in B8634: Diff 12024.Apr 27 2022, 7:43 PM

saw that the S in Apple silicon was lowercase (as it should be)

Off-topic, but Apple themselves are pretty inconsistent about this... eg man arch:

They definitely use "Apple silicon" more often... and that's what we go with in the docs, but figured I'd mention

docs/dev_environment.md
102	Huh surprised about the need to "escape" `.` with a backslash. From what I very vaguely remember `.` and `..` were like shell built-ins...something about directories being files that must have entries for `.` and `..` or something? Guess someone must be aliasing `.` if the `nvm` people are including this, but seems like a crazy thing to do idk.

varun accepted this revision.Apr 28 2022, 12:31 PM

This revision is now accepted and ready to land.Apr 28 2022, 12:31 PM

Closed by commit rCOMM0c97665eb85a: [docs] Fix `nvm` setup instructions (authored by • abosh). · Explain WhyApr 29 2022, 2:54 PM

This revision was automatically updated to reflect the committed changes.

• abosh added a commit: rCOMM0c97665eb85a: [docs] Fix `nvm` setup instructions.

[docs] Fix `nvm` setup instructions
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 12014

docs/dev_environment.md

[docs] Fix `nvm` setup instructionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 12014

docs/dev_environment.md

[docs] Fix `nvm` setup instructions
ClosedPublic
Actions

Revision Contents
Changeset List