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

[develop]: Update ConfigWorkflow chapter to HEAD of develop #915

Merged
merged 183 commits into from
Oct 5, 2023

Conversation

gspetro-NOAA
Copy link
Collaborator

@gspetro-NOAA gspetro-NOAA commented Sep 21, 2023

DESCRIPTION OF CHANGES:

This PR updates the ConfigWorkflow.rst file to align with the current state of config_defaults.yaml in develop.
This PR also adds in-code documentation for undocumented variables in config_defaults.yaml.
There are a few odds & ends in other parts of the documentation to fix errors or make updates that came into develop very recently.

Type of change

  • This change requires a documentation update

TESTS CONDUCTED:

None required. Human-readable version of my fork docs available on RTD: https://srw-ug.readthedocs.io/en/text-config/

DEPENDENCIES:

N/A

DOCUMENTATION:

N/A (all docs)

ISSUE:

Issue #879

CHECKLIST

  • My code follows the style guidelines in the Contributor's Guide
  • I have performed a self-review of my own code using the Code Reviewer's Guide
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes do not require updates to the documentation (explain). N/A
  • My changes generate no new warnings
  • New and existing tests pass with my changes N/A
  • Any dependent changes have been merged and published

Copy link
Collaborator

@RatkoVasic-NOAA RatkoVasic-NOAA left a comment

Choose a reason for hiding this comment

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

Look good to me.

Copy link
Collaborator

@MichaelLueken MichaelLueken left a comment

Choose a reason for hiding this comment

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

@gspetro-NOAA - Thank you for making the previously suggested changes! I've found two spelling issues, and I had a question regarding the VX_NDIGITS_ENSMEM_NAMES variable (should the example show the same values, such as "mem002" to "mem02"). Once these have been addressed, I'll give my approval.

ush/config_defaults.yaml Outdated Show resolved Hide resolved
@MichaelLueken
Copy link
Collaborator

@mkavulich - During the SRW App CM meeting on September 21, you offered to review the documentation updates in this PR. I'm checking to see if you are still planning on reviewing these changes. If you are, then I will hold off on approving and merging this PR until you have had the opportunity to review these changes. Otherwise, I will go ahead, give my approval, and merge this PR. Please let me know as soon as possible whether you would like to review these changes or not. Thank you very much!

@mkavulich
Copy link
Collaborator

@MichaelLueken @gspetro-NOAA apologies for letting this slip! I'm reviewing now.

Copy link
Collaborator

@mkavulich mkavulich 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 this big effort @gspetro-NOAA, this looks like it was a lot of work! Sorry the review took so long, and I admit that I didn't deeply read all the wording changes towards the end, mostly concentrating on the variable names for correctness.

Only a couple of these are corrections, most of these are optional changes so feel free to rebut my suggestions.

Copy link
Collaborator

@mkavulich mkavulich 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 addressing those comments, and opening the new issue #922

Copy link
Collaborator

@MichaelLueken MichaelLueken left a comment

Choose a reason for hiding this comment

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

@gspetro-NOAA - Thanks for working through my last set of suggested changes! Approving now.

@MichaelLueken MichaelLueken linked an issue Oct 5, 2023 that may be closed by this pull request
@MichaelLueken MichaelLueken merged commit 7d2cbfd into ufs-community:develop Oct 5, 2023
3 checks passed
@gspetro-NOAA gspetro-NOAA deleted the text/config branch January 29, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update the SRW App User's Guide to Align w/HEAD of develop
4 participants