Skip to content

Conversation

@jrackham-mo
Copy link
Contributor

@jrackham-mo jrackham-mo commented Jan 30, 2024

Closes #132 and #186.

PR creation checklist for the developer

  • Does the feature branch name follow the format <issue_number>_<short_description_of_feature>?
  • Does the PR title exactly match with the issue title?
  • Have appropriate reviewers been added to the PR (once it is ready for review)?
  • Has the PR been assigned to the developer?
  • Has the Climate Model Evaluation Workflow (CMEW) project been added to the PR?
  • Has the appropriate milestone been added to the PR?

Definition of Done for the developer

  • Does the change in this PR address the above issue / have all acceptance criteria been met?
  • Does the change in this PR follow the recommendations in the wiki: Developer Guide?
  • Have new tests related to the change been added?
  • Do all the GitHub workflow checks pass?
  • Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
  • Has the API documentation related to the change been updated appropriately?
  • Has the user documentation related to the change been updated appropriately?
  • Do the HTML pages render correctly? (See wiki: Build the documentation locally)
  • Do the steps in the Quick Start section of the documentation work?

PR creation checklist for the reviewer

  • Does the feature branch name follow the format <issue_number>_<short_description_of_feature>?
  • Does the PR title exactly match with the issue title?
  • Have appropriate reviewers been added to the PR (once it is ready for review)?
  • Has the PR been assigned to the developer?
  • Has the Climate Model Evaluation Workflow (CMEW) project been added to the PR?
  • Has the appropriate milestone been added to the PR?

Definition of Done for the reviewer

  • Does the change in this PR address the above issue / have all acceptance criteria been met?
  • Does the change in this PR follow the recommendations in the wiki: Developer Guide?
  • Have new tests related to the change been added?
  • Do all the GitHub workflow checks pass?
  • Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
  • Has the API documentation related to the change been updated appropriately?
  • Has the user documentation related to the change been updated appropriately?
  • Do the HTML pages render correctly? (See wiki: Build the documentation locally)
  • Do the steps in the Quick Start section of the documentation work?

@jrackham-mo jrackham-mo added enhancement New feature or request standardise Anything related to CDDS configure Anything related to configuration labels Jan 30, 2024
@jrackham-mo jrackham-mo added this to the v0.1.0 milestone Jan 30, 2024
@jrackham-mo jrackham-mo self-assigned this Jan 30, 2024
@jrackham-mo jrackham-mo linked an issue Jan 30, 2024 that may be closed by this pull request
dasha-shchep
dasha-shchep previously approved these changes Jan 30, 2024
Copy link
Contributor

@dasha-shchep dasha-shchep 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 to me!

@jrackham-mo
Copy link
Contributor Author

The configure_standardise task is now parameterised by assessment area, but it still writes only one variables.txt file. This means that if we introduce multiple assessment areas, then the variables.txt file will get overwritten each time. Perhaps in the future we should name the files more specifically e.g. <assessment_area>_variables.txt. Then we'd also have to modify standardise_model_data to pick up each assessment area's variables file. I imagine we'd have the same issue with the request.json file. For now, we're only dealing with one assessment are, so the issue of reading/writing multiple variables.txt files can be deferred.

@jrackham-mo jrackham-mo marked this pull request as ready for review January 30, 2024 14:04
@jrackham-mo jrackham-mo requested a review from ehogan January 30, 2024 14:05
@ehogan
Copy link
Member

ehogan commented Jan 30, 2024

The configure_standardise task is now parameterised by assessment area, but it still writes only one variables.txt file. This means that if we introduce multiple assessment areas, then the variables.txt file will get overwritten each time. Perhaps in the future we should name the files more specifically e.g. <assessment_area>_variables.txt. Then we'd also have to modify standardise_model_data to pick up each assessment area's variables file. I imagine we'd have the same issue with the request.json file. For now, we're only dealing with one assessment are, so the issue of reading/writing multiple variables.txt files can be deferred.

Would you be happy to open a new issue for this? (Click on the three dots next to your comment and select "Reference in new issue") 😁

@jrackham-mo
Copy link
Contributor Author

Would you be happy to open a new issue for this? (Click on the three dots next to your comment and select "Reference in new issue") 😁

Done #184

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Super stuff, thank you very much @jrackham-mo! 🥳

@jrackham-mo jrackham-mo requested a review from ehogan January 31, 2024 13:51
ehogan
ehogan previously approved these changes Feb 1, 2024
Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Amazing, thank you @jrackham-mo! 🎉

dasha-shchep
dasha-shchep previously approved these changes Feb 1, 2024
@jrackham-mo jrackham-mo dismissed stale reviews from dasha-shchep and ehogan via 6abf99d February 1, 2024 15:48
dasha-shchep
dasha-shchep previously approved these changes Feb 1, 2024
@dasha-shchep
Copy link
Contributor

Add to the original comment that this also closes #186

@jrackham-mo
Copy link
Contributor Author

Add to the original comment that this also closes #186

Updated

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Great work @jrackham-mo and @dasha-shchep! 🥳 I wanted to add that we did discuss doing the last change made on this branch via #186, but we decided that since it was very related to the changes made here that it made sense to make the change via this PR 😊

Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Copy link
Member

@ehogan ehogan 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! 🥳

@jrackham-mo jrackham-mo merged commit 457dbfe into main Feb 2, 2024
@jrackham-mo jrackham-mo deleted the 132_automate_creation_of_variables_list branch February 2, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configure Anything related to configuration enhancement New feature or request standardise Anything related to CDDS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automate the creation of the variables list

4 participants