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

Fix automatic issue closing #1152

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Sep 24, 2020

Fixes issue #: None

Description of the changes being introduced by the pull request:

This pr aims to fix the issue that there are occurrences
when a pr is merged, but the issues that it mentions
that it fixes are not automatically closed by GitHub.

According to their documentation:
https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
Fixes is a keyword that should resolve this.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@MVrachev
Copy link
Collaborator Author

MVrachev commented Sep 28, 2020

PS: As proof that Fixes works on GitHub I noticed in the prs that the last pr by Jussi mirrors: Make targets_path and metadata_path optional where he uses Fixes shows that there is 1 issue linked to the pr:
image

On the other prs it doesn't show that.

@joshuagl
Copy link
Member

I like this change. Not least of all because I often forget to review and close associated issues when merging PRs.

@lukpueh is there a reason we haven't used this auto-closing reference style before now?

@lukpueh
Copy link
Member

lukpueh commented Sep 28, 2020

@lukpueh is there a reason we haven't used this auto-closing reference style before now?

I and others have used them in the past, but it was the responsibility of the PR author to use the add the keyword.

@MVrachev
Copy link
Collaborator Author

Addressed your comments and amended @jku and @lukpueh.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Oct 5, 2020

Thanks to @jku for being skeptical here!

He asked me if I have tried to close automatically an issue with the last variant we all liked:
**Fixes**: #<ISSUE NUMBER>

I decided to have a look and try this in my own private repository.
What I found is that those:

  • **Fixes**: #<ISSUE NUMBER>
  • **Fixes** #<ISSUE NUMBER>

DOESN'T automatically close the issue

what DOES automatically close the issue is to use
Fixes: #<ISSUE NUMBER>

I updated this pr accordingly.

@lukpueh
Copy link
Member

lukpueh commented Oct 5, 2020

Why not just use the documented syntax, i.e. without the colon?

@MVrachev
Copy link
Collaborator Author

MVrachev commented Oct 6, 2020

Why not just use the documented syntax, i.e. without the colon?

It's a personal preference. For me, it looks better with a colon.
It works with a colon and without a colon:

For example, to close an issue numbered 123, you could use the phrase "Closes #123" or "Closes: #123" 
in your pull request description or commit message. 
Once the branch is merged into the default branch, the issue will close.

from https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords#about-issue-references

Should I remove the colon?

@lukpueh
Copy link
Member

lukpueh commented Oct 6, 2020

from https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords#about-issue-references

You are quoting an outdated GitHub Enterprise doc. The one I linked above is the latest and does not mention colons anymore.

Should I remove the colon?

I lean towards yes, but I wouldn't insist. ;)

This pr aims to fix the issue that there are occurrences
when a pr is merged, but the issues that it mentions
that it fixes are not automatically closed by GitHub.

According to their documentation:
https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
"Fixes" is a keyword that should fix this.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Collaborator Author

MVrachev commented Oct 7, 2020

@lukpueh updated the pr with your recommendations and edited both my pr and commit message to reference to the new documentation.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

🙇

@joshuagl joshuagl merged commit ba7c692 into theupdateframework:develop Oct 15, 2020
@MVrachev MVrachev deleted the fix-pr-template branch October 22, 2020 14:18
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.

4 participants