search

NOAA-FIMS / collaborative_workflow

contributors guide to FIMS, managing collaborations
https://noaa-fims.github.io/collaborative_workflow/
4 stars 1 forks source link

docs: change text re the reviewer checklist #73

Closed k-doering-NOAA closed 2 years ago

k-doering-NOAA commented 2 years ago

@kellijohnson-NOAA and I thought this google checklist was more applicable to our group than the checklist in the collaboration guide. We also thought we needed to make the reviewer checklist easy to find, so put together a github action that adds the checklist automatically onto new PRs within the FIMS repo. (See this PR).

kristanblackhart-NOAA commented 2 years ago

@k-doering-NOAA I like the addition of the checklist to PR via github actions, and agree that the workflow document should be aligned with the automatically generated checklist.

Perhaps instead of entirely deleting the existing content as suggested, we could include the checklist that will be automatically generated and then provide some additional context for each item (e.g. what 'good design' entails)? I envision the workflow document as a kind of "one-stop" resource with info beyond what users can find elsewhere.

k-doering-NOAA commented 2 years ago

@kristanblackhart-NOAA , that does make some sense! Kelli and I were worried that it would eventually be out of date relative as to what is in the pull request comment, which is why we didn't add additional content. However, I understand the desire to have one place to look and it does seem like it shouldn't be omitted! Hmm, trying to think how we could sync these up (maybe store text somewhere that both can access?)

kristanblackhart-NOAA commented 2 years ago

The current format is a checklist in the workflow document, which does not match the checklist that is added to the PRs. What if we change the information in the workflow document to read as considerations within each of the major categories to consider when reviewing (e.g. design, readability)? That way there is more context for FIMS stakeholders, but we wouldn't have to worry about text mismatches.

Does that seem like a workable approach @k-doering-NOAA ?

k-doering-NOAA commented 2 years ago

That sounds fine @kristanblackhart-NOAA !

kristanblackhart-NOAA commented 2 years ago

Great. I'll make the updates to the documentation tomorrow and then we can close this out.

ChristineStawitz-NOAA commented 2 years ago

This says it is a work in progress,but I approve whenever you are ready!

k-doering-NOAA commented 2 years ago

@kristanblackhart-NOAA , I think you were going to make some changes, but let me know if there is anything you would like me to do!

kristanblackhart-NOAA commented 2 years ago

@k-doering-NOAA I wasn't able to get to these edits last week during the workshop. I have time blocked this Wednesday to catch up on workflow issues and will address this then - can it wait?

k-doering-NOAA commented 2 years ago

Yes, it can, thanks, Kristan!

kristanblackhart-NOAA commented 2 years ago

@k-doering-NOAA Updates complete to Chapter 7 of the workflow book and closing this. Please reopen if you think further edits are needed. Thanks for your work on this!

k-doering-NOAA commented 2 years ago

Thanks @kristanblackhart-NOAA , I see your changes in 1f4d6ba and agree they make sense! Thanks for spending the time to do this.