Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STORM-2894] fix some random typos #2512

Merged
merged 2 commits into from
Jan 15, 2018
Merged

[STORM-2894] fix some random typos #2512

merged 2 commits into from
Jan 15, 2018

Conversation

erikdw
Copy link
Contributor

@erikdw erikdw commented Jan 10, 2018

  • MultilangEnvirontmentTest
    • There is no "t" between n and m in Environment.
  • getOffsetFromConfigAndFroceFromStart
    • The r and o are transposed in Force.

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@srdo
Copy link
Contributor

srdo commented Jan 12, 2018

+1. Could you squash to one commit before I merge this @erikdw?

@erikdw
Copy link
Contributor Author

erikdw commented Jan 12, 2018

@srdo : I actually spent extra effort to have 2 commits. :-) This was to ensure that there is no hidden changes amidst the renaming of the file (I am a strong proponent of trivial-to-read commits/diffs). With the 2 separate commits, in raw CLI git the changes are easy to follow. When I made the change at first in 1 commit I believe it was hard to see within the git show what lines had been touched. I might be misremembering a bit, but I definitely remember that it was clear in the git status that a non-content-changing rename was done, so long as I didn't touch the file contents simultaneously.

@srdo
Copy link
Contributor

srdo commented Jan 12, 2018

@erikdw Makes sense. I brought it up because I believe at least @hmcl (can't recall if he's the only one) has been requesting that PRs are squashed before merging. Could you explain why @hmcl?

@erikdw
Copy link
Contributor Author

erikdw commented Jan 12, 2018

@srdo : I assume for cleanliness / ease of reading purposes? I am a proponent of squashing commits when there are tons of slight tweaks being made to the same code. I do like separate commits for logically separate things, since it can help understand the code changes in more bite-sized chunks instead of mammoth unreadably-long commits.

@srdo
Copy link
Contributor

srdo commented Jan 12, 2018

@erikdw Yes, I think for cleanliness. Your reason for having two commits makes sense to me. I'll give Hugo a chance to respond, then I'll merge this in a day or two.

@asfgit asfgit merged commit 515b649 into apache:master Jan 15, 2018
@hmcl
Copy link
Contributor

hmcl commented Jan 15, 2018

@erikdw @srdo Thanks for checking in with me. I was off for the last few days and hence just getting back into this, although a bit too late because the patch just got merged before I got the chance to respond.

@erikdw the fundamental reason behind squashing all the commits is sanity. The main challenge when the branches diverge is to track down what happened. The same happens when cherry-picking changes across branches. In Storm this is very hard because Storm's Git log does not have a well​ defined structure, does not have one commit per JIRA, and contributors don't always squash their commits. As an illustration I am adding a the output of "git log" for Storm, Kafka, Spark, Hbase, Flink, and Apex. The difference is notorious. Every other project has a clean, consistent, Git log, which makes it very easy to understand and track down what happened. Furthermore, it makes it also very easy to write git hooks to automate all sorts of tasks to merge and track down what happens cross branches, for example, helping users that have their own private distribution which gets synced with Apache from time to time (I face such tasks periodically and understand how much easier it would be if had a clean git log).

One could argue one way or another. However, a fact is that all of the aforementioned mainstream Apache projects follow one commit per JIRA (with the exception of having perhaps a consistent token such as MINOR), and commits always squashed. In my opinion we should do so as well with absolutely no exceptions. The extra effort to do this is minimal, and the benefits are immense. I have proposed this a few times before, but for some reason it has never been agreed upon or even warranted a lengthy discussion (which honestly is hard for me to understand why).

image

@srdo
Copy link
Contributor

srdo commented Jan 15, 2018

@hmcl I'd raise the discussion on the mailing list, then see if there's at least a lazy consensus. It seems likely to me that a discussion about this will peter out quickly, since some people probably won't care too much, but I think you only need for no one to object to a squash-all policy. If someone really doesn't like the idea, they will -1 and give their reasons. Otherwise I think this falls under non-code change in http://storm.apache.org/contribute/BYLAWS.html.

If no one objects you could put a note about it in the DEVELOPER.md and other places committers are likely to see it.

@HeartSaVioR
Copy link
Contributor

Discussions related to that has been initiating several times, but if my memory is right, some of PMC members were not positive on this.

This was regarding merge script but also contains squashing commits, and there was two -1s from PMC members with mentioning squashing commits.
#1468

But I think things may be changed, so why not initiating another one? We have been discussing and changing legacy steps like removing contributors, removing CHANGELOG, etc. and I think this could be another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants