Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2550
+1.
Thanks @erikdw, it's really good. Thanks also for the discussion. Looking
forward to continue it in the dev list, and hopefully agreeing on a good
solution.
---
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2550
@HeartSaVioR : I just updated all the commit messages in-line with @hmcl's
proposal. I also noticed that I missed the addition of the `serialVersionUID`
to Fields.java so I put that in and rebased.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2550
We may also discuss about rebase (along with squash in this case) vs merge,
and I'm in favor of rebase, because we have huge chance for PR to be reviewed
and merged not immediately. In this case
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2550
> Let me first add just a bit more info to my PR's commit messages per
@hmcl's suggestions.
OK sure. Please let me know when you finished.
> Except that the format of the mails
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2550
> First of all, I need to make this clear: I'm not claiming that this PR
should be changed. I'm OK to merge this in as it is, and I'll do it once 24hrs
policy is met.
Let me first add just a
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2550
@hmcl : now we're going down the rathole that I knew existed if we didn't
take this offline. ð
Before I get to the question of using command-line `git` to see the related
commits, I must
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2550
@erikdw @hmcl @ptgoetz
First of all, I need to make this clear: I'm not claiming that this PR
should be changed. I'm OK to merge this in as it is, and I'll do it once 24hrs
policy is met.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2550
@erikdw I am not sure I follow when you wrote:
> All commits of a PR are bunched together. They can all be referenced as a
group in git revert or git cherry-pick.
In the first git log
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2550
To be clear, I'm largely aligned with what you're both saying. I might not
understand what precise problem you face that makes not squashing such a
problem, but I too would appreciate the aesthetic
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2550
@HeartSaVioR I understand your point of not putting the burden on the
contributors. I didn't really think that it could be a big hurdle for
contributors. However, taking that into consideration it
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2550
@hmcl @HeartSaVioR : perhaps we can take this offline? (I admit to being
guilty of starting this discussion in these PRs.)
I am not grasping the actual problems that you encounter that make
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2550
Other than that I agree most of @hmcl proposed, which is along with the
best practice for what other big projects are following. If we require (not
optional) squashing commits, I don't think
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2550
> The squashing should be done by the creator of the PR and not by the
committer that is going to merge the code.
Sorry but I have been exposing disagreement for this and I still
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2550
@ptgoetz, @srdo and I have been using the following approach for squashing
the patches in storm-kafka-client, which I believe works well:
- It is reasonable to submit a PR for review that has
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2550
@erikdw I agree that in this case where we are cherry-picking several
commits it's very hard to avoid multiple commits, and probably we shouldn't
even try to squash them because that will completely
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2550
+1
---
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/2550
+1 successfully built for me and all tests passed.
As far as squashing commits my opinion has always been if we do squash, it
should be done when merging. There are a lot of cases where
17 matches
Mail list logo
