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

Update radar obs_converter README #306

Merged
merged 4 commits into from Oct 27, 2021

Conversation

johnsonbk
Copy link
Collaborator

@johnsonbk johnsonbk commented Oct 26, 2021

Description:

Update the radar obs_converter README to consolidate Glen's guidance from this issue #37 into the documentation.

Fixes issue

Fixes Github issue #228 .

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

I rebuilt the documentation on my local machine using Sphinx to ensure that this documentation update builds correctly and does not cause any unintended issues.

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Version tag

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

Copy link
Member

@hkershaw-brown hkershaw-brown 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.

One broken link in the list of converters.
obs_converters/README.rst link needs changing to the newfile name:

- `Radar <radar/radar.html>`__
+ `Radar <radar/README.html>`__

Plus, Jeff's comment about adding a link to Alain's pdf.

@johnsonbk
Copy link
Collaborator Author

johnsonbk commented Oct 26, 2021

Two quick questions:

Fix all the obs_converter links?

While I'm editing obs_converters/README.rst should I go ahead and change all of those links in the list of obs_converters into the Sphinx format?

`AIRS <AIRS/README.html>`__
`Aviso+/CMEMS <AVISO/AVISO.html>`__
...
`Var (radar) <var/rad_3dvar_to_dart.html>`__

For example, instead of putting in the HTML link (which is typically used to reference external pages):

`Radar <radar/README.html>`__

It would be the Sphinx :doc: link:

:doc:`radar/README`

The reason that my test build of Sphinx didn't catch the broken link is that it doesn't check for broken html links. It only checks for broken :doc: links.

Okay to omit Alain's PDF in the obs_converter README?

I did some digging. Alain's PDF actually describes the forward operator not the observation converter. We have some documentation of it already:

DART/observations/forward_operators/obs_def_radar_mod.rst

@hkershaw-brown
Copy link
Member

yup both of those make sense

@johnsonbk
Copy link
Collaborator Author

johnsonbk commented Oct 26, 2021

I modified the obs_converter lists in both:

  • guide/available-observation-converters.rst
  • observations/obs_converters/README.rst

While I was at it, I also reformatted the CHAMP README since it contained a broken URL. I can move that commit to a different pull request if that would be cleaner:

  • observations/obs_converters/CHAMP/work/README.rst

@hkershaw-brown hkershaw-brown merged commit 111bda2 into main Oct 27, 2021
@hkershaw-brown hkershaw-brown deleted the radar_obs_converter_update_README branch October 27, 2021 14:54
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.

None yet

2 participants