Update nix installation documentation
to include the install file.
Details
Details
N/A
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This is so much cleaner, I'm only requesting changes to get your feedback on some inline comments. But this looks so much better!
Additionally, @ashoat and @varun should always be listed as reviewers for docs diffs. But looks like @ashoat already saw/gave feedback on this diff. Adding him just in case.
| docs/nix_dev_env.md | ||
|---|---|---|
| 22–24 ↗ | (On Diff #15636) | Could this section could be reworded to match the section in dev_environment.md?  Or vice versa, for consistency? This should be a separate diff/issue, but if Nix can work on Windows we should make it clear that it only works on macOS or Linux for now, like how it's worded on dev_environment.md. |
| 30 ↗ | (On Diff #15636) | I think we should at least hyperlink "Nix package manager" so the developer can have some additional knowledge of what exactly they're downloading. I know we do it on line 9, but lots of devs will skip past the Motivation section and go right to the prerequisites and maybe miss that context. It can be something simple, like the Nix website or any docs you find informational. |
| 33 ↗ | (On Diff #15636) | What are your thoughts on breaking this into two commands? This is totally up to you, like I have no preference. But wanted to suggest this since we use this sort of pattern in dev_environment.md to cd into a directory first and then run related commands. However, this is executing a script, not something like running yarn dev so it may be weird since the user doesn't actually need to be in the scripts directory to run the script. So up to you, but wanted to write this to get your feedback. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 22–24 ↗ | (On Diff #15636) | |
| docs/nix_dev_env.md | ||
|---|---|---|
| 22–24 ↗ | (On Diff #15636) | 😂 I wrote that doc, how ironic |
| docs/nix_dev_env.md | ||
|---|---|---|
| 22–24 ↗ | (On Diff #15636) | This question is unrelated from the diff, but... Given the table above doesn't even list Linux, I don't think we should be telling anybody that the environment is supported on Linux. Maybe we take that part out. At the very least if we want to keep it, we should add a column to the table. |
| 33 ↗ | (On Diff #15636) | I think it's fine as-is |
| docs/nix_dev_env.md | ||
|---|---|---|
| 22–24 ↗ | (On Diff #15636) | Most of my work has been linux compatible. Linux support should only need a quick verification and/or minor work. But until it's a priority, I would say leave it off for now. I believe everyone currently work on Comm is using a mac, so there's not a lot of added value currently. However, since it is OSS, and a majority of which use Linux I think it will eventually be relevant for non-iOS workflows. |
| 30 ↗ | (On Diff #15636) | Yea, that's a fair point |
| 33 ↗ | (On Diff #15636) | I think CWD dependent scripts are an anti-pattern But besides me being particular, I think just running one command instead two which changes your PWD is more desirable. But, this is all subjective. |
Comment Actions
Looks good!
Whether I include @ashoat or not on a diff, he's there. I think it's just a formality at this point :)
I don't think this is true at all. By adding @ashoat as a reviewer, you ensure that the diff pops up in his Phabricator review queue and you follow the rule of always adding @ashoat to a docs diff. This time, @ashoat was on this diff beforehand, but he has to get his eyes on a docs diff before it's landed which is why it's in the Notion doc. You should always add @ashoat and @varun as reviewers to a docs diff before you push your diff.
If you're talking about subscribers to diffs, that's totally different. Like I'm a subscriber on every diff, so I get emails for every diff's updates, but I don't necessarily check or review each diff I get an email for.
Comment Actions
don't think this is true at all
It was meant as a joke :). I know there's a difference, especially with Differential's queue system.