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

Update the contribution guidelines #447

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@massie
Member

massie commented Oct 31, 2014

We don't need to ask people to submit an issue if they already have code to fix the issue.

@massie massie referenced this pull request Oct 31, 2014

Closed

Optimize imports #444

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 31, 2014

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/335/
Test PASSed.

AmplabJenkins commented Oct 31, 2014

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/335/
Test PASSed.

@tdanford

This comment has been minimized.

Show comment
Hide comment
@tdanford

tdanford Oct 31, 2014

Contributor

I'm fine with this. But @massie can you rebase off of master?

Contributor

tdanford commented Oct 31, 2014

I'm fine with this. But @massie can you rebase off of master?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 31, 2014

Member

I am -1 on this. IMO, our project has enough people contributing to it that we should be disciplined about opening issues before writing code. I personally don't think there's much overhead to opening an issue before writing code (or at least before merging code). If we would plan to move to this model after the project grows to a sufficient size anyways, is there any disadvantage to ensuring an issue<->commit link now?

Member

fnothaft commented Oct 31, 2014

I am -1 on this. IMO, our project has enough people contributing to it that we should be disciplined about opening issues before writing code. I personally don't think there's much overhead to opening an issue before writing code (or at least before merging code). If we would plan to move to this model after the project grows to a sufficient size anyways, is there any disadvantage to ensuring an issue<->commit link now?

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Oct 31, 2014

Member

Discipline for the sake of what? What harm to the project are you trying to avoid?

Please tell me the benefit of dogmatically enforcing a 1:1 link between a PR and an issue?

Member

massie commented Oct 31, 2014

Discipline for the sake of what? What harm to the project are you trying to avoid?

Please tell me the benefit of dogmatically enforcing a 1:1 link between a PR and an issue?

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Oct 31, 2014

Member

Issues are great for bringing an issue to everyone's attention, discussing issues before writing code, socializing work planned, organizing releases, etc. We should, of course, use them. What I don't understand it forcing PR contributors to also open an issue and amend their commits to have an issue identified prefix. What does that buy us?

Member

massie commented Oct 31, 2014

Issues are great for bringing an issue to everyone's attention, discussing issues before writing code, socializing work planned, organizing releases, etc. We should, of course, use them. What I don't understand it forcing PR contributors to also open an issue and amend their commits to have an issue identified prefix. What does that buy us?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 31, 2014

Member

@massie the way I see things is, issues and PRs have two separate purposes:

  • Issues should be used to identify that something needs to be worked on (to fix a bug, to expand functionality, etc), and to establish an owner and a timeline for that feature. Issues are useful because they allow us to document the steps for reproducing a bug, where the bug came from, why a feature is necessary, etc, and should allow us to determine whether the issue should be addressed in ADAM or a different (upstream/downstream) repository, and how the issue should be addressed (what does the enhancement/fix look like?).
  • Pull requests are useful for reviewing the actual implementation.

TL;DR: issues are for design review, PRs are for code review.

I think that there is a consensus that in the long term we want all people who are submitting code to ADAM to open an issue before they begin coding a feature. In that model, there would be an issue per commit, no? Do we disagree about this?

If we agree here, the short term benefit of having commits amended with issue IDs is that current commits will be consistent with future commits (i.e., they'll all be tagged with an issue number). If we are consistent about linking commits and issues, it makes it easier to debug regressions, as happened with #353.

From my point of view, I don't really understand why there's resistance to this? I don't think I'm proposing something heavy weight. Since we require people to squash and rebase commits before merging, there's not any tangible overhead to tagging each commit with an issue anyways. We're not even requiring people to open an issue; if it is a small commit (e.g. #437), we're just asking people to amend the commit with the issue (== PR) number so that the commit syntax is consistent across all commits. If I am mistaken, please let me know.

Member

fnothaft commented Oct 31, 2014

@massie the way I see things is, issues and PRs have two separate purposes:

  • Issues should be used to identify that something needs to be worked on (to fix a bug, to expand functionality, etc), and to establish an owner and a timeline for that feature. Issues are useful because they allow us to document the steps for reproducing a bug, where the bug came from, why a feature is necessary, etc, and should allow us to determine whether the issue should be addressed in ADAM or a different (upstream/downstream) repository, and how the issue should be addressed (what does the enhancement/fix look like?).
  • Pull requests are useful for reviewing the actual implementation.

TL;DR: issues are for design review, PRs are for code review.

I think that there is a consensus that in the long term we want all people who are submitting code to ADAM to open an issue before they begin coding a feature. In that model, there would be an issue per commit, no? Do we disagree about this?

If we agree here, the short term benefit of having commits amended with issue IDs is that current commits will be consistent with future commits (i.e., they'll all be tagged with an issue number). If we are consistent about linking commits and issues, it makes it easier to debug regressions, as happened with #353.

From my point of view, I don't really understand why there's resistance to this? I don't think I'm proposing something heavy weight. Since we require people to squash and rebase commits before merging, there's not any tangible overhead to tagging each commit with an issue anyways. We're not even requiring people to open an issue; if it is a small commit (e.g. #437), we're just asking people to amend the commit with the issue (== PR) number so that the commit syntax is consistent across all commits. If I am mistaken, please let me know.

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Nov 1, 2014

Member

I think we agree on the function of issues in the issue tracker and the value of pull requests.

However, we differ in that I maintain not all work needs an issue in the issue tracker or a design review (e.g. cleaning up imports, bumping versions in a pom, etc). I want to make it as easy as possible for people to contribute code without extra process which serves no purpose.

While it makes sense to ask people squash commits and rebase code, I don't see the value of requiring them to always amend the commit with an issue id. In the case where the work needed a design review and issue, then, of course, let's make that part of the workflow.

I'm ok with some commit messages being different than others because some committed code is different than other committed code. We don't need to force homogeneity.

Member

massie commented Nov 1, 2014

I think we agree on the function of issues in the issue tracker and the value of pull requests.

However, we differ in that I maintain not all work needs an issue in the issue tracker or a design review (e.g. cleaning up imports, bumping versions in a pom, etc). I want to make it as easy as possible for people to contribute code without extra process which serves no purpose.

While it makes sense to ask people squash commits and rebase code, I don't see the value of requiring them to always amend the commit with an issue id. In the case where the work needed a design review and issue, then, of course, let's make that part of the workflow.

I'm ok with some commit messages being different than others because some committed code is different than other committed code. We don't need to force homogeneity.

fnothaft added a commit that referenced this pull request Nov 2, 2014

Merge pull request #447 from massie/contribution
Update the contribution guidelines
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Nov 2, 2014

Member

OK, that is fair. I've rebased this and manually merged via 0226b4f.

Member

fnothaft commented Nov 2, 2014

OK, that is fair. I've rebased this and manually merged via 0226b4f.

@fnothaft fnothaft closed this Nov 2, 2014

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Nov 3, 2014

Member

Thanks, Frank!

Member

massie commented Nov 3, 2014

Thanks, Frank!

@massie massie deleted the massie:contribution branch Sep 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment