feature_2054_helpEmailChanges

[davidalbo]

Expected Differences

• Do these changes introduce new tools, command line arguments, or configuration file options? [No]
  
  If yes, please describe:
  

• Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? **[No]*
  
  If yes, please describe:
  

Pull Request Testing

• Describe testing already performed for these changes: None so far
  

• Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions: Some kind of standard tests
  

• Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

• Do these changes include sufficient testing updates? [No]

• Will this PR result in changes to the test suite? [No]
  
  If yes, describe the new output and/or changes to the existing output:
  

• Please complete this pull request review by [2022/02/25].
  

Pull Request Checklist

See the METplus Workflow for details.

• Complete the PR definition above.
• Ensure the PR title matches the feature or bugfix branch name.
• Define the PR metadata, as permissions allow.
  Select: Reviewer(s)
  Select: Organization level software support Project or Repository level development cycle Project
  Select: Milestone as the version that will include these changes
• After submitting the PR, select Linked issues with the original issue number.
• After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
• Close the linked issue and delete your feature or bugfix branch from GitHub.

[jprestop]

@davidalbo Hi Dave. I made some suggested changes. You should be able to just click the "Commit suggestion" button if you agree with the suggested wording and then resolve the conversation by clicking "Resolve Conversation". See screenshot.

[jprestop]

@davidalbo When you're finished with any suggested changes, you can click the circular arrow icon after my name to let me know you're ready for me to re-review. See screenshot. Please let me know if you have any questions.

[davidalbo]

Julie, I believe I went with all your suggested changes. Let me know if not the case. It seems a little 'messy' to me, in the sense that mistakes might be made (by me), as a rookie.

[jprestop]

@davidalbo Some other comments...

I noticed that the boxes in the PR were marked as checked (with the "x" in the brackets, e.g. [x]) under "Pull Request Checklist", but not under "Expected Difference" and "Pull Request Testing" for the PR weren't checked. I went ahead and checked them.

For "Describe testing already performed for these changes:", you said, "None so far". Some things you could have done to see how it looked were to take a look at these lines on these pages to see how your change looked:

MET/met/README

Line 25 in 0f2d6fe

- Create a post in the METplus GitHub Discussions forum: https://github.com/dtcenter/METplus/discussions

MET/met/src/libcode/vx_data2d_grib2/data2d_grib2.cc

Line 707 in 0f2d6fe

<< "Please create a new post with this information in the METplus GitHub Discussions forum at https://github.com/dtcenter/METplus/discussions\n\n";

You could also view the changes in your branch on this page of the documentation:
https://met.readthedocs.io/en/feature_2054_helpemailchange/Users_Guide/appendixA.html

Since GitHub Actions is set up, you didn't really have to do any testing - yay! And, my apologies for not suggesting these things earlier via email. Without see the affected files, it was difficult to come up with these things off the top of my head. It's much easier here with this PR right in front of me.

Under "Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions", since the standard tests are being run automatically with GitHub Actions, you could suggest that the reviewer review the Files Changed and ensure that all of the tests passed.

Instead of answering "No" to "Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation?" and "Do these changes include sufficient testing updates?", I would have said, "Yes" and "N/A". Regarding the documentation updates, you likely would have noticed if you received errors in the documentation from your push to GitHub (you would have received an email saying that Actions failed). Regarding the testing, you didn't need to add any tests so the testing updates question really isn't applicable here.

[jprestop]

@davidalbo Thanks for making the suggested changes! This looks great! Thank you also for requesting the re-review. I approve this pull request. You should be all set to merge your changes, close the linked issue, and delete your feature branch from GitHub. Please let me know if you have any questions and thanks for your work on this task!

[jprestop]

@davidalbo, well, that is, once all of the tests finish running...