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

getting_started/spack.md: incorporate feedback #824

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

harshula
Copy link
Contributor

No description provided.

@harshula harshula self-assigned this Oct 10, 2024
Copy link

github-actions bot commented Oct 10, 2024

PR preview
🛬 Preview link removed because the pull request was closed.

@harshula harshula marked this pull request as draft October 10, 2024 22:34
@harshula harshula force-pushed the harshula/set-up-spack-feedback branch 5 times, most recently from 2c41854 to 5c36fb8 Compare October 14, 2024 06:21
@harshula harshula requested a review from atteggiani October 14, 2024 12:22
@harshula harshula marked this pull request as ready for review October 14, 2024 12:22
Copy link
Contributor

@atteggiani atteggiani left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @harshula.
Overall I like the updates.
There are a couple of things I would change and a few missing coloured outputs (see comments).
I am happy to work on the suggestions/coloured outputs if it's ok for you.

docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
docs/getting_started/spack.md Outdated Show resolved Hide resolved
@harshula harshula force-pushed the harshula/set-up-spack-feedback branch from dd6b6ea to 1e73f67 Compare October 15, 2024 11:06
@harshula harshula requested a review from atteggiani October 15, 2024 13:01
Copy link
Contributor

@blimlim blimlim left a comment

Choose a reason for hiding this comment

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

These instructions look good and are easy to follow!

I've just added a few questions/comments that came up when I went through the instructions. I don't think they're crucial, so feel free to make changes if they seem relevant and skip them if not!

docs/getting_started/spack.md Show resolved Hide resolved
docs/getting_started/spack.md Show resolved Hide resolved
docs/getting_started/spack.md Show resolved Hide resolved
@atteggiani atteggiani marked this pull request as draft October 15, 2024 23:41
@atteggiani atteggiani marked this pull request as ready for review October 15, 2024 23:42
@harshula harshula force-pushed the harshula/set-up-spack-feedback branch 2 times, most recently from dfad5a2 to 51df1a8 Compare October 16, 2024 00:19
@atteggiani atteggiani requested a review from blimlim October 17, 2024 01:47
@atteggiani atteggiani assigned atteggiani and harshula and unassigned harshula Oct 17, 2024
@atteggiani atteggiani requested review from atteggiani and removed request for atteggiani October 17, 2024 01:48
@atteggiani atteggiani requested review from atteggiani and removed request for atteggiani October 17, 2024 01:49
@atteggiani
Copy link
Contributor

Addressed all comments.
@harshula you cannot formally review (because you opened the PR), but please check if this looks good to you now.

Copy link
Contributor

@blimlim blimlim left a comment

Choose a reason for hiding this comment

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

Changes look good to me!

I've just added a small suggestion but don't think it's 100% necessary.

docs/getting_started/spack.md Outdated Show resolved Hide resolved
@harshula
Copy link
Contributor Author

Hi @atteggiani , Looks good. One problem is the spack install output. It's the wrong output.

@atteggiani
Copy link
Contributor

Hi @atteggiani , Looks good. One problem is the spack install output. It's the wrong output.

Fixed the compiler version.

Squashed 10 commits:

getting_started/spack.md: incorporate feedback
* Thanks to Claire Carouge, Chris Bull and Davide Marchegiani.

Changed '/home/565/davide' with '/Users/davide' in 'how to build a model' page

Moved CSS styles to 'access-nri.css'. Minor fixes in the styles for 'how to build a model'.

Added coloured outputs

Fix colours for spack output

Added Spencer's suggestions

Added specification of step not require in admonition of 'how to build a mode' page

Added Spencer's suggestion on liinking the 'how to run a model page'

Fixed compiler in install output

Fixed /Users/davide. Fixed architecture.
@atteggiani atteggiani force-pushed the harshula/set-up-spack-feedback branch from 0c7434c to ae2384b Compare October 17, 2024 05:51
@atteggiani atteggiani merged commit 51278bf into development Oct 17, 2024
6 of 7 checks passed
@atteggiani atteggiani deleted the harshula/set-up-spack-feedback branch October 17, 2024 05:59
@atteggiani
Copy link
Contributor

Thank you @harshula and @blimlim.
Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants