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

#140 - Alternate config_options style #137

Merged
merged 6 commits into from
Sep 26, 2023
Merged

Conversation

ccarouge
Copy link
Collaborator

@ccarouge ccarouge commented Sep 14, 2023

Modifications to config_options.md in the documentation to improve readability:

  • Remove redundant level of heading
  • add a link to sub-headings
  • replace headings for innermost options with links
  • State default values first in the description
  • Reorder options to keep the simpler, required options first
  • Add code example for every single option
  • Add tooltips in examples where useful

@ccarouge ccarouge marked this pull request as draft September 14, 2023 00:24
@ccarouge ccarouge mentioned this pull request Sep 14, 2023
3 tasks
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #137 (efc9b36) into master (44873fc) will increase coverage by 0.29%.
Report is 8 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   88.47%   88.77%   +0.29%     
==========================================
  Files          27       27              
  Lines        1527     1630     +103     
==========================================
+ Hits         1351     1447      +96     
- Misses        176      183       +7     
Files Changed Coverage Δ
benchcab/internal.py 90.47% <ø> (+0.23%) ⬆️

... and 9 files with indirect coverage changes

@SeanBryan51
Copy link
Collaborator

Nice. Can you replace all the options that are headings to

[`option`](#+heading.option){ #+heading.option }

I think it would be best if all the options had a consistent look.

@ccarouge
Copy link
Collaborator Author

Absolutely. This was just a first try to see what it would look like and if it would improve things before going all the way.

@ccarouge ccarouge changed the title #135 - Alternate config_options style #140 - Alternate config_options style Sep 15, 2023
@ccarouge
Copy link
Collaborator Author

Actually, I don't think I want all the options to be links instead of headings. I think we want to keep headings for some of them so they appear in the table of contents. My first try is with only the innermost options as "links" and the other levels of options are headings. I was somewhat taking inspiration from here. It uses headings for all keys that have no values and uses the "link" approach for the sub-keys with values.

Obviously, we have keys at the top level that have values so I'm not sure what will be the best for that case.

@ccarouge
Copy link
Collaborator Author

@SeanBryan51 I'm curious to see what you think of it now.

@SeanBryan51
Copy link
Collaborator

Looks good to me!

@ccarouge ccarouge marked this pull request as ready for review September 22, 2023 00:15
@ccarouge
Copy link
Collaborator Author

@bschroeter Sean and I have seen this documentation way too many times. I think some fresher eyes for the review would be good.

@ccarouge
Copy link
Collaborator Author

@bschroeter you can see a preview of the docs, by clicking on "Details" for the docs/readthedocs.org:benchcab test.

Copy link
Collaborator

@bschroeter bschroeter left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of suggestions from me.

  1. The admonition used to alert the user to all items being required at the top of the page could be the exclamation point (!!! warning). Semantically, an exclamation point usually means "hey, stop, look here!" which is perhaps the message we want to send for that sort of thing. I'm not sure the note icon and colouring conveys the importance and is likely to be overlooked. This does of course change the colour of the box to orange, but perhaps that is a good thing? Note, this isn't actually part of this commit so feel free to ignore this suggestion.

  2. It would be good to have a "Putting it all together" section at the end where all of the options just explained are combined into a complete and working configuration as a code block. This will help users who just want to grab a configuration example and drop it in, without having to worry about getting the yaml syntax correct.

docs/user_guide/config_options.md Outdated Show resolved Hide resolved
@ccarouge
Copy link
Collaborator Author

@bschroeter

The admonition used to alert the user to all items being required at the top of the page could be the exclamation point (!!! warning).

Actually, I think we can completely remove this. I have now specified for each option whether it is optional or required. So that the look is consistent between options. But I've realised I've used required option but optional key. I'll changed all to key.

@ccarouge
Copy link
Collaborator Author

@bschroeter About the "put it all together", we already tell users to start from the bench_example repository. This already contains an example yaml file with all the required options (but not the optional ones). Is that enough? Do you think we should link to it on this doc page? I don't really want to have a copy as they are going to get out of sync.

If people come from the User Guide to this page to see the options, it is linked from this paragraph:

Once the work directory is cloned, you will need to adapt the config.yaml file to your case. Refer to the description of the options for this file.

So they should have the example open on their screen.

- remove admonition box, now redundant
- list experiments in table
- changed "required option" to "required key" for consitency with "optional key"
@bschroeter
Copy link
Collaborator

@bschroeter About the "put it all together", we already tell users to start from the bench_example repository. This already contains an example yaml file with all the required options (but not the optional ones). Is that enough? Do you think we should link to it on this doc page? I don't really want to have a copy as they are going to get out of sync.

If people come from the User Guide to this page to see the options, it is linked from this paragraph:

Once the work directory is cloned, you will need to adapt the config.yaml file to your case. Refer to the description of the options for this file.

So they should have the example open on their screen.

This is a fair assumption. I can appreciate that the example on the docs will likely get out of date with what is installed.

@ccarouge ccarouge merged commit 162f554 into master Sep 26, 2023
3 checks passed
@ccarouge ccarouge deleted the 135-polish-documentation branch September 26, 2023 07:02
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.

3 participants