Delete `./scripts/commit-pr.sh` #1188

Closed
fnothaft opened this Issue Sep 28, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@fnothaft
Member

fnothaft commented Sep 28, 2016

Github has now added a "rebase and merge" button, which does what ./scripts/commit-pr.sh does. I assume that there would be no dissent against deleting the ./scripts/commit-pr.sh script, and switching to use the "rebase and merge" button?

@fnothaft fnothaft added the discussion label Sep 28, 2016

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Sep 28, 2016

Member

I thought we discussed this back when Github first added the Rebase and Merge feature, and there was some argument against it. I can't find documentation of any such conversation (was it during a meeting?) so I'm +1.

Related but slightly offtopic, we may want to update the contributing doc about issue numbers in commit and pull request titles. When there is only one commit to a PR and the PR only references one issue, I've been putting [ADAM-issue#] in the PR title and not the commit message. When the PR covers more than one issue, I've left [ADAM-issue#] from the PR title. If the PR contains more than one commit, I've tried to add [ADAM-issue#] to the commit message. That seems to be roughly the same approach you have been taking? If so, can we codify it in the doc?

In any case, because sometimes PRs are closed via merging and sometimes closed manually, there's no clear way to gather everything for the changelog, which I assume is the point behind the [ADAM-issue#] tagging. See also #936.

Member

heuermh commented Sep 28, 2016

I thought we discussed this back when Github first added the Rebase and Merge feature, and there was some argument against it. I can't find documentation of any such conversation (was it during a meeting?) so I'm +1.

Related but slightly offtopic, we may want to update the contributing doc about issue numbers in commit and pull request titles. When there is only one commit to a PR and the PR only references one issue, I've been putting [ADAM-issue#] in the PR title and not the commit message. When the PR covers more than one issue, I've left [ADAM-issue#] from the PR title. If the PR contains more than one commit, I've tried to add [ADAM-issue#] to the commit message. That seems to be roughly the same approach you have been taking? If so, can we codify it in the doc?

In any case, because sometimes PRs are closed via merging and sometimes closed manually, there's no clear way to gather everything for the changelog, which I assume is the point behind the [ADAM-issue#] tagging. See also #936.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Sep 28, 2016

Member

I thought we discussed this back when Github first added the Rebase and Merge feature, and there was some argument against it. I can't find documentation of any such conversation (was it during a meeting?) so I'm +1.

The trouble with the original feature was that they added squash and merge, not rebase and merge. This week, they added rebase and merge in addition to squash and merge (and plain merge).

Related but slightly offtopic, we may want to update the contributing doc about issue numbers in commit and pull request titles. When there is only one commit to a PR and the PR only references one issue, I've been putting [ADAM-issue#] in the PR title and not the commit message. When the PR covers more than one issue, I've left [ADAM-issue#] from the PR title. If the PR contains more than one commit, I've tried to add [ADAM-issue#] to the commit message. That seems to be roughly the same approach you have been taking? If so, can we codify it in the doc?

+1. I can open a PR for that. My approach is to always start the commit with "[ADAM-issue#]", and to then have "Resolves #issue" in the body of the commit message (which will auto-close the issue once the commit pushes to master).

Member

fnothaft commented Sep 28, 2016

I thought we discussed this back when Github first added the Rebase and Merge feature, and there was some argument against it. I can't find documentation of any such conversation (was it during a meeting?) so I'm +1.

The trouble with the original feature was that they added squash and merge, not rebase and merge. This week, they added rebase and merge in addition to squash and merge (and plain merge).

Related but slightly offtopic, we may want to update the contributing doc about issue numbers in commit and pull request titles. When there is only one commit to a PR and the PR only references one issue, I've been putting [ADAM-issue#] in the PR title and not the commit message. When the PR covers more than one issue, I've left [ADAM-issue#] from the PR title. If the PR contains more than one commit, I've tried to add [ADAM-issue#] to the commit message. That seems to be roughly the same approach you have been taking? If so, can we codify it in the doc?

+1. I can open a PR for that. My approach is to always start the commit with "[ADAM-issue#]", and to then have "Resolves #issue" in the body of the commit message (which will auto-close the issue once the commit pushes to master).

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Sep 28, 2016

Member

The trouble with the original feature was that they added squash and merge, not rebase and merge. This week, they added rebase and merge in addition to squash and merge (and plain merge).

Ah, that's it. Yes, +1 to using the new rebase and merge button.

Member

heuermh commented Sep 28, 2016

The trouble with the original feature was that they added squash and merge, not rebase and merge. This week, they added rebase and merge in addition to squash and merge (and plain merge).

Ah, that's it. Yes, +1 to using the new rebase and merge button.

fnothaft added a commit to fnothaft/adam that referenced this issue Sep 30, 2016

@heuermh heuermh closed this in #1189 Oct 3, 2016

heuermh added a commit that referenced this issue Oct 3, 2016

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