Hi all,
After a short discussion with Pozdnyakov regarding how to accelerating the
current develop-review process, we've reached an agreement and want to share
with you guys, hoping that it can help us from the time-consuming PR
review-and-modify process. It would be great if you could share more ideas.
First of all, we would split the review process into two stages: design and
code details.
During the design review, reviewer will mainly focusing on the overall code
design and point out design issue if any. After that, developers modify the
code if necessary, then ask reviewer to review again. The reviewer should point
out as much issue as possible in order to lower the review-modify loop, and the
developer also should correct all the mistakes (or point out why it should not
be changed) before asking the reviewer to review again.
After the design review is done, the reviewer give "lgtm for the design, let's
move on to the code details" and point out any minor issue in code, again,
reviewer should point out as many issue as possible before asking the developer
to correct them, after it's done, the reviewer will ask the developer to
correct the errors with comments like "LGTM if the issue are fixed." Then
developer fix ALL the issue and ask the reviewer to merge. (or loop back if any
other error is found).
The core idea is that, both developers and reviewers should know each other's
status, and make collective effort to keep the review-modify loop low.
> -----Original Message-----
> From: Balestrieri, Francesco
> Sent: Wednesday, November 13, 2013 9:39 PM
> To: Pozdnyakov, Mikhail; Ming, Bai; OTC PRC Mobile Technology
> Cc: Syrjala, Ilkka
> Subject: RE: About accelerating the development process
>
> Thanks for this discussion, it would be great if you could share your
> suggestions on crosswalk-dev.
>
> Francesco
>
> > -----Original Message-----
> > From: Pozdnyakov, Mikhail
> > Sent: Wednesday, November 13, 2013 11:31 AM
> > To: Ming, Bai; OTC PRC Mobile Technology
> > Cc: Balestrieri, Francesco; Syrjala, Ilkka
> > Subject: RE: About accelerating the development process
> >
> > Hi Bai,
> >
> > >[Ming, Bai] Yes I'm fully agree with you on this point, and so it's
> > >actually kind
> > of unnecessary to spend too much time on the minor issue before we
> > work out a good >design, you know much of modified code may be
> deleted eventually.
> > So, how about we split our review-modify process into two steps,
> > first, we push the code, which >may be WIP but enough for getting a
> > whole picture of the design, then we could start the review -- only
> > focusing on the design until all the following patches are uploaded
> > >and the design is good. Then we could start the next step, to address
> > the minor issue and fix it. We could write some comments in the PR to
> > indicate which step we are >currently in, for example like "okay
> > ,let's start to review the design." Or "The design is okay for me, so
> > let's start to figure out the minor issues" and in the end, "LGTM if
> > >those minor issues are fixed.", and we'll give comments like "Please
> > review the design." Or "I've fixed the minor issues, please have a
> > look if it's good to merge?" I >believe the more we developers and
> reviewers know about each other's progress, the more efficient we would
> be.
> > > Wdyt?
> >
> > Yeah, this is exactly what I mean. Looks good to me :) especially for
> > big features. Thanks.
> >
> > BR,
> > Mikhail
> >
> > ________________________________________
> > From: Ming, Bai
> > Sent: Wednesday, November 13, 2013 4:21 AM
> > To: Pozdnyakov, Mikhail; OTC PRC Mobile Technology
> > Cc: Balestrieri, Francesco; Syrjala, Ilkka
> > Subject: RE: About accelerating the development process
> >
> > > -----Original Message-----
> > > From: Pozdnyakov, Mikhail
> > > Sent: Tuesday, November 12, 2013 6:51 PM
> > > To: Ming, Bai; OTC PRC Mobile Technology
> > > Cc: Balestrieri, Francesco; Syrjala, Ilkka
> > > Subject: RE: About accelerating the development process
> > >
> > > Hi Bai,
> > >
> > > I really appreciate your intention to introduce a process so that
> > > reviewing is more efficient and less time-consuming.
> > > I agree with your point: all the essential issues should be pointed
> > > out ASAP during the initial review and then the corrected code won't
> > > take too much time before merging (it is supposed to contain only
> > > minor issues). I promise to follow this process as much as I can.
> > [Ming, Bai] Thank you!
> > >
> > > I should admit however that sometimes it is pretty hard to follow
> > > this procedure, especially if patch contains implementation of a new
> > > feature: it might take several iterations to figure out an
> > > appropriate design for the new functionality. Finding of a good
> > > design is the most important part of review
> > > IMO: unlike minor issues which are easy to fix later, correcting of
> > > an inappropriate design might require rewriting of the whole module
> > > (or even whole project!). Initially badly designed code is always
> > > causing more badly written code to come (hacks are always needed!)
> > > and resulting in a huge pile of unmaintainable code.
> > >
> > > Apparently, finding of an appropriate design is sometimes making
> > > review process quite painful for both a developer and a reviewer but
> > > it is surely a necessary step. In order to make it easier I can
> > > recommend to upload WIP patches (having minor issues,
> unimplemented
> > > functionality but mature enough to see which object is doing what
> > > (and using which interfaces)) and discuss it with reviewers. This
> > > might prevent the necessity to rewrite the whole fully-implemented
> > > patch. What do
> > you think?
> > >
> > [Ming, Bai] Yes I'm fully agree with you on this point, and so it's
> > actually kind of unnecessary to spend too much time on the minor issue
> > before we work out a good design, you know much of modified code may
> > be deleted eventually. So, how about we split our review-modify
> > process into two steps, first, we push the code, which may be WIP but
> > enough for getting a whole picture of the design, then we could start
> > the review -- only focusing on the design until all the following
> > patches are uploaded and the design is good. Then we could start the
> > next step, to address the minor issue and fix it. We could write some
> > comments in the PR to indicate which step we are currently in, for
> > example like "okay ,let's start to review the design." Or "The design
> > is okay for me, so let's start to figure out the minor issues" and in
> > the end, "LGTM if those minor issues are fixed.", and we'll give
> > comments like "Please review the design." Or "I've fixed the minor
> > issues, please have a look if it's good to merge?" I believe the more we
> developers and reviewers know about each other's progress, the more
> efficient we would be.
> > Wdyt?
> >
> > > BR,
> > > Mikhail
> > >
> > > ________________________________________
> > > From: Ming, Bai
> > > Sent: Tuesday, November 12, 2013 9:04 AM
> > > To: Pozdnyakov, Mikhail; OTC PRC Mobile Technology
> > > Subject: About accelerating the development process
> > >
> > > Hi Pozdnyakov,
> > > Thank you for reviewing our runtime model patches, it's really
> > > helpful for us to address the issue in our code quickly and efficiently.
> > > So, it must took you a lot of time to go through these patches over
> > > and over, you know, my last patch regarding the shared runtime
> > > process model, we've worked on it for a week. What I'm considering
> > > is that, it's a little bit time- consuming for you to review the
> > > same patch multiple times, and also not very efficient in getting the
> patch to be merged.
> > > Perhaps a better develop-review method could help both of us from
> > > this kinda tedious process, what do you think? so, here is the deal:
> > > The current process is like this:
> > > Step 1, we developers should do a thorough analysis on the patch and
> > > make sure it can pass the Trybot.
> > > Step 2, we do a cross-review in our own team to eliminate minor errors.
> > > Step 3, we ask you to have a review on our patch.
> > > Step 4, you review the patch and give comments.
> > > Step 5, we correct the issues and loop back to step 3.
> > > Step 6, Merge the patch.
> > > Among the steps, most of our time were consumed on the 'loop'
> > > between step 3 to step 5, which I think, could be optimized somehow.
> > > My suggestion is: Within one loop, we correct the code as much as we
> > > could, and you pick up the errors as many as you could.
> > > Step 3, we ask you to have a review.
> > > Step 4, you pick up the errors as much as you could, perhaps it will
> > > take you several days and it's okay, so it would be better to do a
> > > estimate and inform us beforehand.
> > > Step 5, We start to correct the error and it may also take some
> > > time, we will inform you about the estimated time.
> > > Step 6, you review only the corrected code, point out is there is any
> > > error.
> > > Step 7, we correct the error and loop back to 6.
> > > Step 8 merge.
> > > Perhaps it's unlikely to find out all the errors in the first round,
> > > so I think it's okay to loop back to step 4 one or two times.
> > > How does that sound?
> > >
> > >
> > > Best regards!
> > > --
> > > Bai
> > >
> > >
> > >
> > >
> > >
> > >
> > > --
> > > - Ming, Bai
_______________________________________________
Crosswalk-dev mailing list
Crosswalk-dev@lists.crosswalk-project.org
https://lists.crosswalk-project.org/mailman/listinfo/crosswalk-dev
