[DISCUSS] Submitting small PRs rather than massive ones

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] Submitting small PRs rather than massive ones

Henry Saputra
Hi All,

Recently there have been some PRs with massive changes which include
multiple JIRA tickets.

It is getting tougher to review and also to back port changes if needed.

To help reviewers to help review the changes lets try to submit small
but often PRs to make it easier to review.
Not to mention Github UI suffers with diff changes over 200 files and
thousands lines of code changes =)

When committing to ASF git it should be fine to combine one day of
work but PRs should as isolated as possible.

Exception such as new module like Gelly or ML maybe ok, but others
that require changes to the execution flow should be done if smaller
batches if possible.

Thanks,

Henry
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

Stephan Ewen
I like this proposal very much. We should do that as much as possible.

Pull requests with renaming easily add up to many files, it is harder there.
Am 18.03.2015 19:39 schrieb "Henry Saputra" <[hidden email]>:

> Hi All,
>
> Recently there have been some PRs with massive changes which include
> multiple JIRA tickets.
>
> It is getting tougher to review and also to back port changes if needed.
>
> To help reviewers to help review the changes lets try to submit small
> but often PRs to make it easier to review.
> Not to mention Github UI suffers with diff changes over 200 files and
> thousands lines of code changes =)
>
> When committing to ASF git it should be fine to combine one day of
> work but PRs should as isolated as possible.
>
> Exception such as new module like Gelly or ML maybe ok, but others
> that require changes to the execution flow should be done if smaller
> batches if possible.
>
> Thanks,
>
> Henry
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

Ufuk Celebi-2

On 19 Mar 2015, at 09:43, Stephan Ewen <[hidden email]> wrote:

> I like this proposal very much. We should do that as much as possible.

Same here. Makes it also easier to track progress.

(I think this should go hand in hand with better design descriptions in the corresponding JIRAs.)
mxm
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

mxm
I agree with you, Henry. Reviewing hundreds of changes class files is a
difficult and a nearly impossible task to do exhaustive.

However, splitting up pull requests also has some drawbacks. For example,
discussions and comments are also split up and harder to keep up with.
Also, pull requests might depend on other pull requests.

Therefore, I would advise to make use of Gits power and split up pull
requests into as many logical commits as possible. Individual commits can
be reviewed just like individual pull requests. The advantage being that
they can build on each other.

Max

On Thu, Mar 19, 2015 at 10:17 AM, Ufuk Celebi <[hidden email]> wrote:

>
> On 19 Mar 2015, at 09:43, Stephan Ewen <[hidden email]> wrote:
>
> > I like this proposal very much. We should do that as much as possible.
>
> Same here. Makes it also easier to track progress.
>
> (I think this should go hand in hand with better design descriptions in
> the corresponding JIRAs.)
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

Henry Saputra
In reply to this post by Stephan Ewen
Yeah, renaming totally will contain large file changes.

On Thu, Mar 19, 2015 at 1:43 AM, Stephan Ewen <[hidden email]> wrote:

> I like this proposal very much. We should do that as much as possible.
>
> Pull requests with renaming easily add up to many files, it is harder there.
> Am 18.03.2015 19:39 schrieb "Henry Saputra" <[hidden email]>:
>
>> Hi All,
>>
>> Recently there have been some PRs with massive changes which include
>> multiple JIRA tickets.
>>
>> It is getting tougher to review and also to back port changes if needed.
>>
>> To help reviewers to help review the changes lets try to submit small
>> but often PRs to make it easier to review.
>> Not to mention Github UI suffers with diff changes over 200 files and
>> thousands lines of code changes =)
>>
>> When committing to ASF git it should be fine to combine one day of
>> work but PRs should as isolated as possible.
>>
>> Exception such as new module like Gelly or ML maybe ok, but others
>> that require changes to the execution flow should be done if smaller
>> batches if possible.
>>
>> Thanks,
>>
>> Henry
>>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

Henry Saputra
In reply to this post by Ufuk Celebi-2
+1 to  that, Ufuk.

Making JIRA more descriptive and contain design would make it better
to review b4 jumping to the diff in the PRs.

On Thu, Mar 19, 2015 at 2:17 AM, Ufuk Celebi <[hidden email]> wrote:
>
> On 19 Mar 2015, at 09:43, Stephan Ewen <[hidden email]> wrote:
>
>> I like this proposal very much. We should do that as much as possible.
>
> Same here. Makes it also easier to track progress.
>
> (I think this should go hand in hand with better design descriptions in the corresponding JIRAs.)
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Submitting small PRs rather than massive ones

Henry Saputra
In reply to this post by mxm
Great suggestion and observation Max.

+1 Yes, we should also splitting PRs into right and logical commits
that will definitely help with review.
Like you said some PRs are just large in nature and splitting it into
pieces may not work well so doing right commits should go hand in hand
with small PRs as necessary.

When I mean by smaller PRs is to make sure one PR try to solve one
logical issue at a time.
If we can split it into smaller PRs, by designing your solution
propoerly, it would help history management better. Think of it as one
agile sprint story =)

- Henry



On Thu, Mar 19, 2015 at 2:27 AM, Maximilian Michels <[hidden email]> wrote:

> I agree with you, Henry. Reviewing hundreds of changes class files is a
> difficult and a nearly impossible task to do exhaustive.
>
> However, splitting up pull requests also has some drawbacks. For example,
> discussions and comments are also split up and harder to keep up with.
> Also, pull requests might depend on other pull requests.
>
> Therefore, I would advise to make use of Gits power and split up pull
> requests into as many logical commits as possible. Individual commits can
> be reviewed just like individual pull requests. The advantage being that
> they can build on each other.
>
> Max
>
> On Thu, Mar 19, 2015 at 10:17 AM, Ufuk Celebi <[hidden email]> wrote:
>
>>
>> On 19 Mar 2015, at 09:43, Stephan Ewen <[hidden email]> wrote:
>>
>> > I like this proposal very much. We should do that as much as possible.
>>
>> Same here. Makes it also easier to track progress.
>>
>> (I think this should go hand in hand with better design descriptions in
>> the corresponding JIRAs.)