git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: 'Design Review' tag


I think we should encourage contributors to supply design proposal before
giving PR.   When contributors have supplied their PRs ,they have
implemented their codes by their original design ideas.  Giving `Design
Review` is usually too late to them ,and let them feel disappointed. So we
should guide them to give design proposal thorough the dev emails , JIRAs
or github issues.

On Sun, Oct 28, 2018 at 7:01 AM Jihoon Son <ghoonson@xxxxxxxxx> wrote:

> Roman, thanks for the detailed explanation. I totally agree with it.
>
> I would add something more here for especially emphasizing the part of
> 'design review'.
> As Roman said, the PRs tagged with 'Design Review' usually include the
> changes which are hard to undo, so, it's very important to make the best
> decision we can.
> And the best design decision can be found by only considering reasonable
> evidences.
>
> So, I would suggest to enforce writing a proposal for all PRs which should
> be labelled 'Design Review'. We're currently doing it only if someone wants
> to write, but it gives a lot of benefits as below:
>
> - To write a detailed proposal, authors can have a chance to think about
> their idea thoroughly including very details like APIs, configuration
> names, or even some code level changes.
> - We can have a chance that authors provide enough evidences for every
> design decision, so that reviewers can check and verify them. I believe
> that we can make the best decision in this process.
> - Since it's only a proposal not containing any code changes yet, it's much
> easier to change design decisions than changing codes.
>
> Many other open source projects are also doing this. For example, Kafka has
> the 'Kafka Improvement Proposals' system (
>
> https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals
> ).
> For Rust, it needs to write an RFC for substantial changes (
> http://rust-lang.github.io/rfcs/).
>
> I think one of the reasons why people hesitate to write proposals in Druid
> is the absence of the appropriate format. I think the format should include
> at least the followings:
>
> - The problem description and motivation
> - Proposed change
>   - Overview of the change
>   - Details including public API changes, configuration changes, algorithm,
> and so on
> - Expected benefits and drawbacks
> - Rationale and alternatives
>
> Welcome any idea.
>
> Best,
> Jihoon
>
> On Fri, Oct 26, 2018 at 11:03 PM Roman Leventov <leventov@xxxxxxxxxx>
> wrote:
>
> > Recently Charles (as far as I remember) asked what is the criteria for
> > giving a PR 'Design Review' tag.
> >
> > I suggest that *a PR should be labelled "Design Review" if it will be
> hard
> > to undo it (after it appears in some release), it will have lasting
> > consequences on the codebase.*
> >
> > Addition of a new config property, public API, or changing existing
> public
> > API is always hard to undo because it immediately becomes a subject for
> > backwards compatibility. But some large code changes, even without
> > user-facing API or config changes, could have similar effect on the
> > codebase.
> >
> > Please keep in mind that it's not just for requiring two reviews instead
> of
> > one, but also for really reviewing the design. E. g. bad config
> > key/property names could stay forever because it's really hard to rename
> > them, even harder than change Java APIs.
> >
>