On 28.11.2019 01:56, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Lars Kurth wrote:
>> +This could take for example the form of
>> +> Do you think it would be useful for the code to do XXX?
>> +> I can imagine a user wanting to do YYY (and XXX would enable this)
>> +
>> +That potentially adds additional work for the code author, which they may not have
>> +the time to perform. It is good practice for authors to consider such a request in terms of
>> +* Usefulness to the user
>> +* Code churn, complexity or impact on other system properties
>> +* Extra time to implement and report back to the reviewer
>> +
>> +If you believe that the impact/cost is too high, report back to the reviewer. To resolve
>> +this, it is advisable to
>> +* Report your findings
>> +* And then check whether this was merely an interesting suggestion, or something the
>> +reviewer feels more strongly about
>> +
>> +In the latter case, there are typically several common outcomes
>> +* The **author and reviewer agree** that the suggestion should be implemented
>> +* The **author and reviewer agree** that it may make sense to defer implementation
>> +* The **author and reviewer agree** that it makes no sense to implement the suggestion
>> +
>> +The author of a patch would typically suggest their preferred outcome, for example
>> +> I am not sure it is worth to implement XXX
>> +> Do you think this could be done as a separate patch in future?
>> +
>> +In cases, where no agreement can be found, the best approach would be to get an
>> +independent opinion from another maintainer or the project's leadership team.
>
> I think we should mention somewhere here that it is recommended for
> reviewers to be explicit about whether a request is optional or whether
> it is a requirement.
>
> For instance: "I think it would be good if X also did Y" doesn't say if
> it is optional (future work) or it is actually required as part of this
> series. More explicit word choices are preferable, such as:
>
> "I think it would be good if X also did Y, not a requirement but good to
> have."
>
> "I think it would be good if X also did Y and it should be part of this
> series."
I think without it being made explicit that something is optional,
the assumption should be that it isn't. I.e. in the first example
I agree with the idea to have something after the comma, but in
the second example I think the extra wording is a waste of effort.
> I think there is something else we should say on this topic. There is a
> category of things which could be done in multiple ways and it is not
> overtly obvious which one is best. It is done to the maintainer and the
> author personal styles. It is easy to disagree on that.
>
> I think a good recommendation would be for the contributor to try to
> follow the maintainers requests, even if they could be considered
> "style", trusting their experience on the matter. And a good
> recommendation for the maintainer would be to try to let the contributor
> have freedom of implementation choice on things that don't make a
> significant difference.
I think we try to, but I also think we suffer from too little
clear documentation on e.g. style aspects. Attempts on my part
to address this have mostly (not entirely) lead no-where (lack of
feedback on proposed patches to ./CODING_STYLE). So for the time
being there are (many) aspects where we have de-facto expectations
that aren't written down anywhere, with the result of (in a subset
of cases) disagreement on what the perceived de-facto standard
actually is.
Jan
_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
> On Thu, 26 Sep 2019, Lars Kurth wrote:
>> +This could take for example the form of
>> +> Do you think it would be useful for the code to do XXX?
>> +> I can imagine a user wanting to do YYY (and XXX would enable this)
>> +
>> +That potentially adds additional work for the code author, which they may not have
>> +the time to perform. It is good practice for authors to consider such a request in terms of
>> +* Usefulness to the user
>> +* Code churn, complexity or impact on other system properties
>> +* Extra time to implement and report back to the reviewer
>> +
>> +If you believe that the impact/cost is too high, report back to the reviewer. To resolve
>> +this, it is advisable to
>> +* Report your findings
>> +* And then check whether this was merely an interesting suggestion, or something the
>> +reviewer feels more strongly about
>> +
>> +In the latter case, there are typically several common outcomes
>> +* The **author and reviewer agree** that the suggestion should be implemented
>> +* The **author and reviewer agree** that it may make sense to defer implementation
>> +* The **author and reviewer agree** that it makes no sense to implement the suggestion
>> +
>> +The author of a patch would typically suggest their preferred outcome, for example
>> +> I am not sure it is worth to implement XXX
>> +> Do you think this could be done as a separate patch in future?
>> +
>> +In cases, where no agreement can be found, the best approach would be to get an
>> +independent opinion from another maintainer or the project's leadership team.
>
> I think we should mention somewhere here that it is recommended for
> reviewers to be explicit about whether a request is optional or whether
> it is a requirement.
>
> For instance: "I think it would be good if X also did Y" doesn't say if
> it is optional (future work) or it is actually required as part of this
> series. More explicit word choices are preferable, such as:
>
> "I think it would be good if X also did Y, not a requirement but good to
> have."
>
> "I think it would be good if X also did Y and it should be part of this
> series."
I think without it being made explicit that something is optional,
the assumption should be that it isn't. I.e. in the first example
I agree with the idea to have something after the comma, but in
the second example I think the extra wording is a waste of effort.
> I think there is something else we should say on this topic. There is a
> category of things which could be done in multiple ways and it is not
> overtly obvious which one is best. It is done to the maintainer and the
> author personal styles. It is easy to disagree on that.
>
> I think a good recommendation would be for the contributor to try to
> follow the maintainers requests, even if they could be considered
> "style", trusting their experience on the matter. And a good
> recommendation for the maintainer would be to try to let the contributor
> have freedom of implementation choice on things that don't make a
> significant difference.
I think we try to, but I also think we suffer from too little
clear documentation on e.g. style aspects. Attempts on my part
to address this have mostly (not entirely) lead no-where (lack of
feedback on proposed patches to ./CODING_STYLE). So for the time
being there are (many) aspects where we have de-facto expectations
that aren't written down anywhere, with the result of (in a subset
of cases) disagreement on what the perceived de-facto standard
actually is.
Jan
_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api