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

Adapt ESMValTool to new configuration #3761

Merged
merged 34 commits into from
Oct 22, 2024
Merged

Adapt ESMValTool to new configuration #3761

merged 34 commits into from
Oct 22, 2024

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Oct 2, 2024

Description

This PR adapt all parts of ESMValTool to the new configuration (see ESMValGroup/ESMValCore#2448).

Deprecation

Similar to the corresponding ESMValCore PR, this PR deprecates the command line option --config_file for esmvaltool data format/download/prepare. This option is scheduled for removal in version 2.14.0. Please use the option config_dir instead.

Example:

Old syntax:

esmvaltool data format --config_file=~/my/config/file.yml WOA

New syntax:

esmvaltool data format --config_dir=~/my/config/ WOA

See ESMValGroup/ESMValCore#2448 and ESMValGroup/ESMValCore#2371 for further details.


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number of pull requests:

@schlunma schlunma added the requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. label Oct 2, 2024
@schlunma schlunma added this to the v2.12.0 milestone Oct 2, 2024
@schlunma schlunma self-assigned this Oct 2, 2024
@schlunma schlunma added deprecated feature and removed requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. labels Oct 7, 2024
@schlunma schlunma marked this pull request as ready for review October 7, 2024 13:18
@schlunma schlunma requested a review from a team as a code owner October 7, 2024 13:18
#. Download the data following the instructions included in the script and place
it in the ``RAWOBS`` path specified in your ``config-user.yml``
#. Download the data following the instructions included in the script and
place it in the ``RAWOBS`` `rootpath` specified in your :ref:`configuration
Copy link
Member

Choose a reason for hiding this comment

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

Use double backticks around rootpath to make it monospace (a single backtick is italic and the convention is to use monospace for things that have to be typed exactly like that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I am always unsure about this. Numpy recommends to enclose parameter names in single backticks, and they seem to be aware of the different meanings of single backticks in markdown and reST. I've also seen the usage of italic font for parameters in Numpy's (and other project's) API documentation (e.g., bins here). That's why I used single backticks here.

However, I can totally see your point about the double backticks. As long as we use one convention consistently (and we're currently certainly not doing that), I guess both are fine? So maybe we should just once and for all decide on one and continue to use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in ce62d9f and 75e2b33

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I assumed our convention was to use monospace font for things that need to be written exactly like that to work and all the other cases were people who just forgot that in .rst you need double backticks to create monospace fonts. Maybe we need to write this in our contributor docs somewhere too. From the text you linked it looks like numpy plans to adopt the same convention, but are working on some feature to make that work with single backticks:

This guide continues to recommended that parameter names be enclosed within single backticks. Currently, this may cause parameter names to render improperly and cause warnings, but numpydoc will soon release a feature that causes them to render as monospaced hyperlinks to the parameter documentation.

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Nice work! Just a few comments on improving the formatting and possibly removing some unnecessary code from the recipe filler.

Copy link
Member

@bouweandela bouweandela 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 making all the changes @schlunma! I believe there is one comment still outstanding: #3761 (comment), could you have a look?

@schlunma
Copy link
Contributor Author

Thanks for making all the changes @schlunma! I believe there is one comment still outstanding: #3761 (comment), could you have a look?

Done in f0e7565

@bouweandela bouweandela merged commit 8f7982c into main Oct 22, 2024
7 of 8 checks passed
@bouweandela bouweandela deleted the new_config branch October 22, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation build is broken
2 participants