Policy for handling patches which need further design #72400

New Issue

Campbell Barton · 2019-12-12T23:54:23+01:00

Campbell Barton commented

2019-12-12 23:54:23 +01:00

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.

Occasionally we run into situations where a patch is submitted which may be useful but has design implications which need further investigation. Examples: - [D6387: View3D: Frame All if Frame Selected is called with no selection](https://archive.blender.org/developer/D6387) - [D6313: Add panning to circle select](https://archive.blender.org/developer/D6313) 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.

Campbell Barton commented

2019-12-12 23:54:23 +01:00

Added subscriber: @ideasman42

Benjamin Sauder commented

2019-12-13 12:14:26 +01:00

Added subscriber: @kioku

Evan Wilson commented

2019-12-13 22:19:53 +01:00

Added subscriber: @EAW

Ankit Meel commented

2020-08-16 13:42:13 +02:00

Added subscriber: @ankitm

Ankit Meel commented

2020-08-16 13:42:13 +02:00

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.

>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 commented

2021-02-11 18:50:35 +01:00

Added subscriber: @Sergey

Sergey Sharybin commented

2021-02-11 18:50:35 +01:00

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.

`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 commented

2021-02-17 11:51:48 +01:00

Added subscriber: @dfelinto

Dalai Felinto commented

2021-02-17 11:51:48 +01:00

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.

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 commented

2021-02-17 12:38:52 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2021-02-17 12:38:52 +01:00

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

Ryan Inch commented

2021-03-04 06:52:32 +01:00

Added subscriber: @Imaginer

Ryan Inch commented

2021-03-04 06:52:32 +01:00

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?

> 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 commented

2021-03-04 10:14:42 +01:00

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 commented

2021-03-13 09:23:53 +01:00

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.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Policy for handling patches which need further design #72400

Proposal