bug#71622: mumi CLI client features for review checklists

bug#71622: mumi CLI client features for review checklists

[Arun Isaac]

The mumi CLI must provide features to help go through a review
checklist. A system that allows even those without commit access to
contribute meaningfully to the review process could be a big win. We can
use this issue to brainstorm ideas. One idea could be as follows.

# Idea 1

Projects provide a review checklist in their .mumi/config. For example,
something like

((review-checklist . (((name . good-commit-message)
                       (description . "Are the commit messages written well?")
                       (tag . review-good-commit-message))
                      ((name . good-synopsis-description)
                       (description . "Are the synopsis and description written well?")
                       (tag . review-good-synopsis-description))
                      ((name . tests-run)
                       (description . "Are the package tests being run (if available)?")
                       (tag . review-tests-run))
                      […]))
 […])

When a reviewer checks one of these items (say the good-commit-message),
they run something like
$ mumi review --tick good-commit-message
and that sets the review-good-commit-message tag on the issue.

We could also have a status command like
$ mumi review --status
that lists the complete checklist with a tick mark by items that have
been checked.

This system is really a convenience wrapper around tagging. So, it can
be searched with something like
$ mumi search tag:review-good-commit-message
One might however argue that such searching is not very useful.

One possible downside is that this ties each project (guix, mumi, etc.)
to a single checklist. For example, what about guix patches that are not
for packages? Perhaps it is an idea to allow multiple checklists per
project.

Another downside is that this does not provide for multiple reviewers to
review and verify each other's findings. In other words, there is no way
for two reviewers to register that they both verified something
independently.

# Idea 2

A second much simpler idea is to implement templates for `mumi
compose'. Projects provide templates under .mumi/templates. For example,
$ cat .mumi/templates/review
[ ] Are the commit messages written well?
[ ] Are the synopsis and description written well?
[ ] Are the package tests being run (if available)?

Then, when reviewers review something, they compose an email like so
$ mumi compose --template review
that composes an email with this template. The reviewer puts an 'x' by
items they have checked.

The downside of this method is that this is just unstructured
text. There is no way for mumi to understand what parts of the review
checklist have been completed and thus generate useful reports,
filtering, etc.

Thank you for listening to my brain dump! Suggestions welcome.

Cheers! :-)

bug#71622: mumi CLI client features for review checklists

[Suhail Singh]

Arun Isaac <arunisaac@systemreboot.net> writes:

> # Idea 1
>
> Projects provide a review checklist in their .mumi/config.

I think idea #1 can add considerable value.  I'll note some opinionated
observations below.

> For example, something like
>
> ((review-checklist . (((name . good-commit-message)
>                        (description . "Are the commit messages written well?")
>                        (tag . review-good-commit-message))
>                       ((name . good-synopsis-description)
>                        (description . "Are the synopsis and description written well?")
>                        (tag . review-good-synopsis-description))
>                       ((name . tests-run)
>                        (description . "Are the package tests being run (if available)?")
>                        (tag . review-tests-run))
>                       […]))
>  […])

I think the fact that the names are distinct from the actions (the tag
that is to be added) is quite important.  I.e., the "review-checklist"
should provide a convenience around "well-defined state transitions"
where the state is being recorded by usertags, but could also be
extended to include issue status (open, close, owner etc).

For instance as noted in
<https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#4._Set_yourself_as_the_reviewer>:

- When setting yourself as the reviewer, ensure that no-one else has set
  themselves as the owner.
- Assuming above is true, when adding under-review tag, also add
  yourself as the owner.

On a related note, perhaps "review-workflow" might be a better name than
"review-checklist"?

> When a reviewer checks one of these items (say the good-commit-message),
> they run something like
> $ mumi review --tick good-commit-message
> and that sets the review-good-commit-message tag on the issue.

It may be important that when a tag is set, that others are unset.  It
would help if this were possible to express via the review-checklist.
For instance as noted in
<https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#12._Change_the_issue_state>:

There are three possible transitions after a review is completed:
- Addition of reviewed-looks-good, removal of under-review .
- Addition of escalated-review-request, removal of under-review .
- Addition of waiting-on-contributor, removal of under-review .

The user-interface (as well as the configuration) should focus on those
transitions.  Some transitions may indeed be simple and may only result
in the addition of a single tag, whereas others may be more complex.
Additionally, given the current state, only some transitions may be
"sensible" and permissible, i.e., it would help to be able to express
conditional/guarded transitions.

> One possible downside is that this ties each project (guix, mumi,
> etc.)  to a single checklist.

I think that that's actually a benefit.  It can aid review of the
transition rules for them to be in one place.

> For example, what about guix patches that are not for packages?
> Perhaps it is an idea to allow multiple checklists per project.

One way to address this would be to have the ability to have multiple
conditional "checklists" (or workflows) that are mutually exclusive and
exhaustive.  I.e., support for nested "checklists" in addition to
guarding would be sufficient, but may not be necessary.

> Another downside is that this does not provide for multiple reviewers to
> review and verify each other's findings. In other words, there is no way
> for two reviewers to register that they both verified something
> independently.

Could that be addressed by ensuring that the control messages are also
cc'd to the specific issue?

-- 
Suhail

bug#71622: mumi CLI client features for review checklists

[Ludovic Courtès]

Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> # Idea 1
>
> Projects provide a review checklist in their .mumi/config. For example,
> something like
>
> ((review-checklist . (((name . good-commit-message)
>                        (description . "Are the commit messages written well?")
>                        (tag . review-good-commit-message))
>                       ((name . good-synopsis-description)
>                        (description . "Are the synopsis and description written well?")
>                        (tag . review-good-synopsis-description))
>                       ((name . tests-run)
>                        (description . "Are the package tests being run (if available)?")
>                        (tag . review-tests-run))
>                       […]))
>  […])

[...]

> # Idea 2
>
> A second much simpler idea is to implement templates for `mumi
> compose'. Projects provide templates under .mumi/templates. For example,
> $ cat .mumi/templates/review
> [ ] Are the commit messages written well?
> [ ] Are the synopsis and description written well?
> [ ] Are the package tests being run (if available)?

I prefer #2: it’s familiar to anyone used to Git{Hub,Lab}, it’s simpler,
good enough, and doesn’t go too far to provide a technical solution to a
social problem.

Alongside, a ‘mumi approve’ command that would set the
‘reviewed-looks-good’ tag would be a great step forward.

Thank you!

Ludo’.