-
Notifications
You must be signed in to change notification settings - Fork 256
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
Refresh installation and build docs #700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- File name contains typo: buildLing.rst
Thanks for the review @Julian-O. Feedback addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cmacdonald !
I've left some comments. Some of them are not really tied to your PR, but makes sense to update what looks outdated, while we're moving things.
Let me know what you think about it.
Co-authored-by: Mirko Galimberti <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @misl6. Thanks for your review. I addressed those changes.
I read over the building.rst in more details, adding my own thoughts about duplicated material and possible better structuring, and other thoughts about the material.
Thanks @misl6. I vastly simplified the building page. Screenshot here for ease of reference: Perhaps the Visual Studio link could be inline like the rest? Did I remove anything important? |
Quick style review of screenshot:
|
I don't care enough to suggest a further change, but both Windows and Linux build environments have "environment variables", but only Linux (and Unix) use the term "shell" so when you talk about "shell environment variables" being needed on Windows, it doesn't match up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Agree with @Julian-O regarding env variables, but I also do not want to lock this PR progress. Feel free to address it separately.
This addresses the changes discussed in #695.