Code review process needs revamping

[zepumph]

From https://github.com/phetsims/chipper/issues/1180

@samreid had some great thoughts about how challenging it was to conduct a code review because of the length and formatting of the CRC. During our discussion about where to put the typescript-specific conventions over in https://github.com/phetsims/chipper/issues/1180, we decided that keeping a separate document made sense. Perhaps the best path to fix the CRC more generally would be to split out pieces that take up a lot of space, and have a lot of content, and if something in it fails, to create another issue where that can be annotated. Members attending the dev meeting today agreed.

We should discuss this more in next dev meeting, because we ran out of time, but as a proof-of-concept, the "Coding Conventions" section can be easily moved out to another document. I'll do that

@samreid: This feels like something that will bleed into future quarters, and that is OK!

[zepumph]

coding conventions are now in their own document. Note that over in https://github.com/phetsims/chipper/issues/1180 I will be pushing to combine the coding-conventions and the typescript-conventions document.

To reiterate, I heard two sentiments during the conversation today that seemed like ways to tangibly improve the code review experience:

• [ ] Modularize the checklist such that certain, lengthy sections could become separate issues, (perhaps only if they failed the CRC in some way?)
• [ ] Rewrite checkboxes, if easy, such that when you click the button to "promote checkbox to issue", the title of the issue wasn't overly longwinded.

In addition to these, some time could be devoted (since we are on the topic), to more extreme revamping ideas. None come to mind currently, but I wanted to leave the door open!

Marking for dev meeting.

[zepumph]

Note that over in https://github.com/phetsims/chipper/issues/1180 I will be pushing to combine the coding-conventions and the typescript-conventions document.

I will do that here too! I just found that a decision we made about how to export variables was documented in the tyepscript conventions after discussion in https://github.com/phetsims/phet-info/issues/184, but no changes were made to the document: https://github.com/phetsims/phet-info/blob/master/doc/best-practices-for-modules.md which is used in the CRC. we have too many different conventions documents, and they should be combined.

[samreid]

From today's dev meeting conversation:

@chrisklus identifies that it is awkward to use the long monolithic checklist, and it would be good to modularize. @samreid: One "code review" issue with one comment per section? Or numerous issues, one per section? @kathy-phet and @chrisklus say one issue seems good. @zepumph says it could be developer discretion.

Consensus: We would like to have a more modular code review process.

Question: Should the "typescript conventions" doc be more like a checklist with checkboxes?

@kathy-phet asked for volunteers in breaking up the large code review checklist. @zepumph asks whether this should be more about typescript?
@kathy-phet: Since @jonathanolson is doing a code review now, what do you recommend?

@jonathanolson: I'll review the typescript conventions doc and make recommendations. I'll look through my review comments and see if things need to be added. The review is https://github.com/phetsims/geometric-optics/issues/402. I'll also check the latest version of the TypeScript conventions doc to see if there is more for the review.

@zepumph: Can you please also glance through the main code review checklist, to see if other parts can be pulled out to modules? Are there typescript-specific changes our team has made that would warrant other changes to the code review checklist?

@jonathanolson says: sounds good.

@zepumph asks if anyone wants the TypeScript conventions reads more like a checklist? @jonathanolson: It's nice not being a checklist. Going through that long checklist, some parts are nice to be checked off. But for style guide and recommendations, it would be a hassle for each item to be too checklisted. @zepumph do you want the main code review doc to be less checklisty? @jonathanolson: Personally, I don't feel like that checklist is benefiting me. But maybe it is more helpful for others.

@kathy-phet: Maybe more junior members would benefit more from the checklisty aspects. @chrisklus: @Luisav1 pruned it down and used it like a checklist. I can check in with her about that experience.

@zepumph: The "coding conventions" doc is a checklist, but "typescript conventions" is not.

@samreid: The main problem with checking off the large list, is it takes 5 seconds or so for a checkbox to check. @zepumph: I just do that in Sublime. I do feel like it's nice to have individual items.

We agreed to make this a subgroup centered on @jonathanolson and then we can discuss whether we want to keep making it more modular.

[zepumph]

• [ ] We also didn't conclude the discussion of if the coding conventions doc should lose its checkbox structure. We felt like the way that typescript conventions is set up, and that is preferred for many devs.

[marlitas]

This seems like it might be related to https://github.com/phetsims/phet-info/issues/192 and the work the subgroup is doing there. Bringing back to to be discussed to see if reassignment is beneficial for this issue.

[marlitas]

12/22/22 JO: a style guidelines document being separate from the main code review checks would be the most helpful. CM: There is a wide range of needs for the code review checklist. I do not go through the list box by box, however for those new to the crc process here that may be useful. JO: checkboxes take a while to go through. KP: Perhaps parsing this out into a style guide

We will continue discussing this in January, we ran out of time.