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

Adapting Ravenbrook and MM group review procedure to public MPS #123

Draft
wants to merge 107 commits into
base: master
Choose a base branch
from

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Jan 19, 2023

Introduces Memory Pool System review procedure in order to solve #101 .

Work stages:

  1. Review all the documents mentioned in MPS documentation lacks a review procedure #101 (comment) and copy the relevant parts of their text to the procedure, in order to capture all the process improvement value in those.
  2. Update the documents to use more current language ("pull request", "issue comment", etc.) and current tools (Git, GitHub).
  3. Test the procedure.
  4. Subject the procedure to itself.

@rptb1 rptb1 requested review from thejayps and UNAA008 January 21, 2023 15:57
@rptb1 rptb1 added git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo process To do with process or procedure labels Jan 21, 2023
@rptb1 rptb1 linked an issue Jan 21, 2023 that may be closed by this pull request
@rptb1
Copy link
Member Author

rptb1 commented Feb 3, 2023

The second test run of procedure at #83 (comment) went smoothly. No real problems found in the procedure, but a few good suggestions.

Note for inclusion: Base kickoff, check,log, and brainstorm time seems to be about 2h elapsed for the team we have. The procedure itself suggests a limit of 3.5h. Express review is capped at 30m.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Consider renaming "review" to "inspection" for this particular process:

  • it's more accurate
  • it doesn't co-opt the word "review" which is useful for lots of other things
  • it's less likely to confuse people used to other kinds of review
  • it makes the point to outsiders that we're doing something more than the usual review
  • it might break references from older material a bit, including the old MM stuff.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Some thoughts about how to apply this document to itself, when it comes to it. It's about 3600 lines of text, so that's 6h at the baseline 10 lines/min. However, it's quite reviewable in sections, especially since most of the procedures a made to be done separately.

Consistency with the introduction and rationale is important. And the overall structure of the document for its purpose will need attention, possibly both at the start (to see how it appeals to someone new approaching it) and at the end (when everyone's read it all).

We can also switch tactics after one or two sessions.

@rptb1
Copy link
Member Author

rptb1 commented Feb 24, 2023

In #119 (review) @thejayps mentioned design/guide.review.txt, guide.impl.c.format, and guide.impl.c.naming . These are Ravenbrook-era review documents that need absorbing/merging into rules and checklists.

@rptb1 rptb1 self-assigned this Feb 26, 2023
@thejayps
Copy link
Contributor

thejayps commented Nov 13, 2023

Who is responsible for assigning non checking roles (eg scribe, pi) and when does this happen?

Currently it is not happening on some reviews. This is related to #274

@thejayps
Copy link
Contributor

After looking at "something random" in weekly meeting: https://info.ravenbrook.com/rule/ was found as a source document for review of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Will cause failure of the entire project git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo process To do with process or procedure
Projects
Development

Successfully merging this pull request may close these issues.

MPS documentation lacks a review procedure
2 participants