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

Add binaries for crashme feature and update documentation. Fix #32 #33

Merged
merged 1 commit into from Mar 13, 2012

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Mar 12, 2012

Talked with Ted on IRC and there is nothing which would prevent us from doing so.

tonymec added a commit that referenced this pull request Mar 13, 2012
Add binaries for crashme feature and update documentation. Fix #32
@tonymec tonymec merged commit 9c7bdf6 into mozilla:master Mar 13, 2012
@whimboo
Copy link
Contributor Author

whimboo commented Mar 13, 2012

Thanks Tony for merging the pull request. Just a notice for you. In the future we want that others comment on pull requests only and make notes about what has to be changed or if everything is ok. The owner should take care of merging the code into master. That also implies that we rebase all the commits into a single one before we merge.

If you are unsure about some specifics you can leave the merge work to me.

@tonymec
Copy link
Contributor

tonymec commented Mar 13, 2012

On 13/03/12 15:16, Henrik Skupin wrote:

Thanks Tony for merging the pull request. Just a notice for you. In the future we want that others comment on pull requests only and make notes about what has to be changed or if everything is ok. The owner should take care of merging the code into master. That also implies that we rebase all the commits into a single one before we merge.

If you are unsure about some specifics you can leave the merge work to me.

I thought that the reason you hadn't merged it yourself immediately
after requesting the pull was that for some reason you wanted an
additional "pair of eyes" on that pull. It looked quite straightforward
to me so I felt it was OK for me to merge it. I almost added "r=tonymec"
but finally left it out as I wasn't sure such an addition was in order.

I intentionally added no additional commit message to what github
generated by default.

I'm not sure what you mean by "rebase all the commits into a single one
before we merge". In this case there was only one commit to be merged,
and github said there was no conflict requiring human arbitration. In
fact the reason I left you the merge work a week or so ago was that I
wasn't sure which of xabolcs's pull requests really deserved merging,
and in what order. (I hadn't followed the corresponding bugs very closely.)

Best regards,

Tony.

Microsoft's definition of a boolean: TRUE, FALSE, MAYBE
"Embrace and extend"...?

@whimboo
Copy link
Contributor Author

whimboo commented Mar 14, 2012

Frankly no-one should merge the own code immediately. There should always happen a review by at least one person. Only that way we can make sure to keep the quality and everyone of us can learn given by the review comments. That's why I haven't merged it - and will even not do it in trivial cases.

If there was only a single commit then it's fine. It was more an addition to things we have to check before getting pull requests merged in the future. It will give us a cleaner commit history over time and doesn't add clutter for interim commits.

So I propose that we let the owner merge his own code after a finished review, or if he/she doesn't have the permissions the final reviewer is doing it.

Does it make sense? If not which other way would you propose? A discussion on it we probably should move off into a mail thread.

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

Successfully merging this pull request may close these issues.

None yet

2 participants