Maniphest T72400

Policy for handling patches which need further design
Confirmed, NormalDESIGN
Actions

Assigned To
None
Authored By
Campbell Barton (campbellbarton)
Dec 12 2019, 11:54 PM
Tags
  • BF Blender
Subscribers
Ankit Meel (ankitm)
Bastien Montagne (mont29)
Benjamin Sauder (kioku)
Campbell Barton (campbellbarton)
Dalai Felinto (dfelinto)
Evan Wilson (EAW)
Ryan Inch (Imaginer)
Sergey Sharybin (sergey)
Tokens
"Like" token, awarded by zazizizou."Like" token, awarded by EAW."Like" token, awarded by harley.

Description

Occasionally we run into situations where a patch is submitted which may be useful but has design implications which need further investigation.

Examples:

This complicates review.

  • Rejecting the patch forces us to make the argument against the functionality (which can be disputed), drawing the reviewer into further discussion, defending points... etc.
  • While opening a design task for the patch is an option, it means we need to devote time to evaluate the changes (implications for the future, ensure it's applied consistently, possible alternatives... etc).
  • Leaving the patch open isn't great since it's ignoring the contribution.

Proposal

  • Patches which have merit but may not fit well into the current design are closed (linking to a summary of the notes above).
  • A link to this patch is added to the module page under a section for this purpose.
  • Module teams are responsible for reviewing these item (it may only be a few each release), and either opening a design task when time permits, or closing.

This way contributions are not blocked by individual developers or left open indefinitely. If nobody in the module-team thinks is worth spending time on - we can make the decisions and let the developer know which a short explanation.

Related Objects

Event Timeline

Campbell Barton (campbellbarton) created this task.Dec 12 2019, 11:54 PM
Campbell Barton (campbellbarton) mentioned this in T71640: Development Policies.
Campbell Barton (campbellbarton) updated the task description.Dec 12 2019, 11:58 PM
Campbell Barton (campbellbarton) updated the task description.
Campbell Barton (campbellbarton) edited a custom field.
Campbell Barton (campbellbarton) updated the task description.Dec 13 2019, 12:02 AM
Harley Acheson (harley) awarded a token.Dec 13 2019, 1:12 AM
Evan Wilson (EAW) awarded a token.Dec 13 2019, 1:34 AM
Benjamin Sauder (kioku) added a subscriber: Benjamin Sauder (kioku).Dec 13 2019, 12:14 PM
Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).Dec 13 2019, 10:19 PM
Habib Gahbiche (zazizizou) awarded a token.Dec 24 2019, 10:56 AM
Campbell Barton (campbellbarton) mentioned this in D8286: Node Editor Link Drag Search Menu.Jul 19 2020, 11:47 PM
Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).Aug 16 2020, 1:42 PM

Module teams are responsible for reviewing these item (it may only be a few each release), and either opening a design task when time permits, or closing.

Can "plan changes" (and not abandoning) be used when a design task is present ? A downside is that reviewer/moderator cannot take this action, so it requires reminding the contributor.

Sergey Sharybin (sergey) added a subscriber: Sergey Sharybin (sergey).Feb 11 2021, 6:50 PM

Plan Changes indeed seems the striaghtforward communication. The issue is: I think it is restricted to the author to this state, so either need a policy to request author to plan changes, or to allow non-authors to move patch to this state.

Dalai Felinto (dfelinto) added a subscriber: Dalai Felinto (dfelinto).EditedFeb 17 2021, 11:51 AM

I agree with the sentiment, but for the solution I would like to propose:

  • A new phabricator tag Needs Approved Design. (or a patch subtype)
  • All queries ignore patches that have this tag.
  • Once a design task (submitted by someone willing to implement the feature) is submitted and approved the reviewer manually lift the tag of the associated patch.
  • Periodically we can manually close patches that have no associated design task.
  • If the design associated with the patch is rejected, the patch is closed.
Bastien Montagne (mont29) added a subscriber: Bastien Montagne (mont29).Feb 17 2021, 12:38 PM

Agree with the general idea here, do not care really about how this is practically implemented in phabricator (@Dalai Felinto (dfelinto)'s proposal sounds good though)

Ryan Inch (Imaginer) added a subscriber: Ryan Inch (Imaginer).Mar 4 2021, 6:52 AM

All queries ignore patches that have this tag.
Once a design task (submitted by someone willing to implement the feature) is submitted and approved the reviewer manually lift the tag of the associated patch.

If all queries ignore it, how will anyone know to create a design task for it?

Dalai Felinto (dfelinto) added a comment.Mar 4 2021, 10:14 AM

While doing "Periodically we can manually close patches that have no associated design task" the tasks that got a design task can have that flip. But I was mainly talking about the existing module queries. The triaging team can help looking at new queries specifically for that. Besides whoever tagged it will receive an email/notification once the patch is updated with the associated design task.

Ryan Inch (Imaginer) added a comment.Mar 13 2021, 9:23 AM

Perhaps I'm not understanding you correctly. It sounded in your proposal like random devs were supposed to find these and, if interested, create design tasks so that the patches could move forward; however, having queries ignore patches would seem to impede random devs finding them.