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 ?
Thanks,
Boaz
[1]
https://stackoverflow.com/questions/1440050/how-to-split-last-commit-into-two-in-git/1440200
[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 <[email protected]>
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
