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

Added instructions on contributing and requesting changes #132

Merged
merged 4 commits into from Mar 28, 2017

Conversation

d-stahl-ericsson
Copy link
Contributor

As per issue #131.

1. Anyone can participate in reviews, not only Eiffel team members.
1. A Pull Request should be approved by at least two Eiffel team members (including the one doing the merging). For this to function well, the above point on participation is critical.
1. Do not feel any pressure to merge Pull Requests. Unless you feel confident about what you are doing, don't press that big green button. Instead, ask a more senior member to make the decision.
1. When squashing and merging, ensure that the description reflects the change. Detailing every individual commit in the Pull Request is unnecessary, as they are squashed anyway. Instead, describe the change as a single thing. That description should always include an Issue reference. A lot of the time a descriptive title and an Issue reference is all that is needed.
Copy link

Choose a reason for hiding this comment

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

Minor improvement: Also state that the description should state what you sort of get when you include the commit (compared not including the commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't follow...?

Choose a reason for hiding this comment

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

It is the what vs. how dimension. I have seen a few too many descriptions on the level of what has changed, but nothing on why.

See bullet 7 https://chris.beams.io/posts/git-commit/ for a more general discussion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm with you. Actually that's a pretty good guide, didn't read that one before (but the xkcd strip is brilliant!).

To emphasize the need of providing context, rather than just
listing changes.
1. Anyone can participate in reviews, not only Eiffel team members.
1. A Pull Request should be approved by at least two Eiffel team members (including the one doing the merging). For this to function well, the above point on participation is critical.
1. Do not feel any pressure to merge Pull Requests. Unless you feel confident about what you are doing, don't press that big green button. Instead, ask a more senior member to make the decision.
1. When squashing and merging, ensure that the description reflects the change. Detailing every individual commit in the Pull Request is unnecessary, as they are squashed anyway. Instead, describe the change as a single thing. That description should always include an Issue reference, and should focus on WHY a the change was made, to provide the reader with context. See [this excellent guide](https://chris.beams.io/posts/git-commit) on writing good commit messages.

Choose a reason for hiding this comment

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

"and should focus on WHY a the change was made"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh! Fixed.

@e-backmark-ericsson
Copy link
Member

👍

@d-stahl-ericsson
Copy link
Contributor Author

I'll go ahead and merge this tomorrow, unless anyone objects.

@d-stahl-ericsson d-stahl-ericsson merged commit d9de812 into eiffel-community:master Mar 28, 2017
@d-stahl-ericsson d-stahl-ericsson deleted the issue131 branch September 13, 2018 10:31
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

3 participants