drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boaz Ben-Zvi <b...@apache.org>
Subject Multi Commit PRs (Re: Drill Hangout tomorrow 09/18)
Date Wed, 26 Sep 2018 02:34:52 GMT
    More on splitting a PR into multiple commits - link [1] below shows 
how to take the last commit and break it (thanks Hanumath.)

I just practiced this method on a PR (1480 - see [2]); this separates 
the actual logic of the change from the less relevant definitions, 
cleanups, etc.

This does require careful manual work from the developer; like if two 
changes are adjacent (i.e. become a single "hunk"), then you need to 
select the "e" option and edit that "hunk".

An open question: Must we eventually squash those multiple commits, or 
would it work better to keep them apart committed into the master ?




[2] https://github.com/apache/drill/pull/1480/commits

On 9/24/18 1:49 PM, Jyothsna Reddy wrote:
> Notes from the Hangout session
> Attendes:
> Jyothsna, Boaz, Sorabh, Arina, Bohdan, Ihor, Hanumath, Pritesh, Vitali,
> Kunal, Robert
> Interesting thing shared by Boaz : All the minor fragments are assigned to
> Drillbits in round robin fashion and not in a sequential order.
> Boaz brought up the topic of improving the quality of code reviews:
> Topic of the Hangout: How do we improve the process of code review?
> It is highly difficult for the reviewer to do a code review if he/she
> doesn't know the context and hard to figure out if the PR contains too many
> code changes.
> Ideas to improve the code review process:
>     - One idea is to break the commits into smaller commits so that each
>     commit is coherent and keeping the refactoring changes in a different
>     commit. But its hard for the developers to separate out into multiple
>     commits if they are too deeply tangled. Although it creates more work for
>     developers, it makes reviewers job easier by doing this. This helps in
>     finding bugs in earlier stages too.
>     - It would be easier if someone can find ways where Git allows to split
>     the commits. Hanumath had tried this earlier.
>     - Mandating check style before code review and it shouldn't be code
>     reviewer's job to point out those.
>     - Bring a reviewer early on into code review process rather than dumping
>     a 10000 line code changes at a go.
>     - Push smaller commits into master if they make sense.
>     - Do some live code review sessions where external contributors and
>     reviewer can have discussions related to pull requests in a hangout.
>     - Don't squash the commits unless needed.
>     - Reviewers should give full set of comments at one go and there
>     shouldn't be more than 4-5 rounds of code reviews.
>     - Check style should be included for spaces and stuff and developers
>     should try to use IntelliJ IDE and should pay attention to the warnings.
>     - Its helpful for reviewers if developers provide screenshots of UI for
>     UI changes and attach before and after code if changes are made to code
>     generators.
> Please feel free to add ideas to the above incase if you have any ideas to
> improve the code review process.
> Thank you,
> Jyothsna
> On Mon, Sep 17, 2018 at 12:55 PM Jyothsna Reddy <jyothsna.dvj@gmail.com>
> wrote:
>> The Apache Drill Hangout will be held tomorrow at 10:00am PST; please let
>> us know should you have a topic for tomorrow's hangout. We will also ask
>> for topics at the beginning of the hangout.
>> Hangout Link -
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__hangouts.google.com_hangouts_-5F_event_ci4rdiju8bv04a64efj5fedd0lc&d=DwIBaQ&c=cskdkSMqhcnjZxdQVpwTXg&r=7lXQnf0aC8VQ0iMXwVgNHw&m=9AQiac0o0ILqquFD8t1gtRKb9VgnUsNWPhyNGEa7x4Q&s=tNdr_LHgocB7NB3XiSCrp296AMXJgG7YHuOaKD95X74&e=
>> Thank you,
>> Jyothsna

View raw message