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 a how-to guide section #6926

Merged
merged 8 commits into from Apr 20, 2023
Merged

Add a how-to guide section #6926

merged 8 commits into from Apr 20, 2023

Conversation

wtbarnes
Copy link
Member

@wtbarnes wtbarnes commented Apr 17, 2023

Please rebase and merge. Do not squash merge.

Fixes #6909
Fixes #5427

I've converted one entry from the example gallery to a how-to guide which I thought was a fairly uncontroversial example. I'd like to limit this PR to just adding the section/general structure and then leave a larger restructuring of other content to how-to guides to other PRs. A debate on what in the example gallery should be moved here would tie this up indefinitely.

I'd mostly like to decide on naming, formatting, high-level text and general wording for the descriptions of the sections here. Once we have this merged, we can start reorganizing more asynchronously.

@wtbarnes wtbarnes marked this pull request as ready for review April 17, 2023 23:02
@wtbarnes wtbarnes requested a review from a team as a code owner April 17, 2023 23:03
@nabobalis nabobalis added this to the 5.0.0 milestone Apr 17, 2023
docs/how_to/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@nabobalis nabobalis added Documentation Affects the documentation Needs Review Needs reviews before merge No Changelog Entry Needed labels Apr 18, 2023
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Two suggestions:

For the map stuff, I've left inline comments. Essentially I think having code that loads a map into a variable at the top, then using that variable in the code snippets below would be a bit clearer.

I think some words could be saved by splitting the table into different sections. For e.g. the map section, there could be a "Maps" title at the top, and then the text could change like:

  • "create a map from a FITS file" > "load from a FITS file"
  • "save a map to a FITS file" > "save to a FITS file"
  • "get a quicklook summary of a map" > "get a quicklook"

removing all the "a map" bits would make it easier to read IMO

docs/how_to/index.rst Outdated Show resolved Hide resolved
docs/how_to/index.rst Outdated Show resolved Hide resolved
@wtbarnes
Copy link
Member Author

Essentially I think having code that loads a map into a variable at the top, then using that variable in the code snippets below would be a bit clearer.

I initially avoided this because I was directly linking to the docstring, but since I'm constructing the links manually anyway, I think this does make more sense.

I think some words could be saved by splitting the table into different sections.

I disagree. I think the table should be as simple as possible and as short as possible, particularly since these code snippets are not tested. I'm worried that if it starts increasing in complexity and length, we'll just be duplicating the API docs and the table will lose its purpose.

@dstansby
Copy link
Member

👍 - I still definietely thing there are words to be cut down for brevity/readability, but I'm happy to bikeshed that in a future PR instead of here.

@wtbarnes
Copy link
Member Author

👍 - I still definietely thing there are words to be cut down for brevity/readability, but I'm happy to bikeshed that in a future PR instead of here.

Agreed. I made a pass at making it a bit less wordy and avoiding repetition.

@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Apr 20, 2023
@nabobalis nabobalis requested a review from dstansby April 20, 2023 02:55
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👏

@dstansby dstansby merged commit 31ba6ae into sunpy:main Apr 20, 2023
25 checks passed
@wtbarnes
Copy link
Member Author

🎉 We now have spaces for all four diataxis sections!

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

Successfully merging this pull request may close these issues.

Add a "How-to Guide" section to the docs Add a "How do I..." page to our documentation
3 participants