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

Add clarifying language to docs index page #93

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Conversation

paigem
Copy link
Contributor

@paigem paigem commented Mar 6, 2024

In general, I think the documentation of this package could be made a bit clearer. I make a few suggestions in this PR, particularly around the language used to describe the 3 examples. However, I haven't used NEMO data myself, so please review the wording I used and ensure that it is correct.

My main issue with this docs index page is that it assumes a high level of NEMO knowledge. I understand that most people who use this tool will be well-versed in the output files and structures of NEMO, but with just a little effort, these docs can also be helpful for people who are new to using NEMO.

For example, I don't actually know why it's important to "recombine" the domain_cfg and mesh_mask files (Example 2). I took a stab at explaining why this is necessary in this PR, but just a sentence motivating why this is useful for users would be very helpful.

Copy link
Owner

@rcaneill rcaneill left a comment

Choose a reason for hiding this comment

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

Hi @paigem, I updated your suggestions. If you're happy with them, I let you commit them.

@rcaneill
Copy link
Owner

rcaneill commented Mar 6, 2024

(PS I fixed the pre-commit hook in another PR so no need to worry that the action is failing)

Copy link
Contributor Author

@paigem paigem left a comment

Choose a reason for hiding this comment

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

Thanks for your wording suggestions - I have accepted them all. I think this will be much clearer for users!

@paigem
Copy link
Contributor Author

paigem commented Mar 7, 2024

Thanks @rcaneill for your input here! I don't think I have the ability to merge, so I'll let you do the honors when you're ready.

Copy link
Contributor Author

@paigem paigem left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying the wording here - looks good to me!

@rcaneill rcaneill merged commit 61309d3 into rcaneill:main Mar 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add context in documentation around how this package fits in with existing packages
2 participants