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

Delete "Functions should do one thing" section #170

Merged

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Sep 18, 2019

Decided to delete section Functions should do one thing #38 (comment)


EDIT by @TomasVotruba : Closes #38

@TomasVotruba
Copy link
Contributor

Thanks! Just a tip for next PR to auto-close original issue on merge:

https://help.github.com/en/articles/closing-issues-using-keywords

@TomasVotruba TomasVotruba merged commit 8693a08 into piotrplenik:master Sep 18, 2019
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Sep 18, 2019

Thank you 👍

@peter-gribanov
Copy link
Contributor Author

Thanks! Just a tip for next PR to auto-close original issue on merge:

https://help.github.com/en/articles/closing-issues-using-keywords

@TomasVotruba I know this, but it doesn’t always work 😕

@peter-gribanov peter-gribanov deleted the delete_functions_one_thing branch September 19, 2019 10:04
@TomasVotruba
Copy link
Contributor

What exactly doesn't work?

"Close X" in the body of PR always worked for me for last 4 years

@peter-gribanov
Copy link
Contributor Author

@TomasVotruba Usually works, but sometimes not. Just a week ago there were a couple of such cases.

I try to always mark the closing ticket, but sometimes i forget to do it. Sorry for this.

@TomasVotruba
Copy link
Contributor

No troubles!

Ping me next time it doesn't work, I'm curious about it

@peter-gribanov
Copy link
Contributor Author

peter-gribanov commented Nov 8, 2019

@TomasVotruba you were interested in an example.

Bugreport created
Happyr/Doctrine-Specification#232
2 bugfix PR were created.
First rejected
Happyr/Doctrine-Specification#233
Second merged
Happyr/Doctrine-Specification#234
Both PR have a link to this issue.
image
This issue did not close automatically after PR merged.
Such situations rarely occur, but they do.

image

@TomasVotruba
Copy link
Contributor

Thanks for feedback.
I usually use "fixes" or "closes", not "fix". Maybe "fix" is only working in some cases. That's my random tip.

@rob006
Copy link

rob006 commented Nov 8, 2019

This issue did not close automatically after PR merged.
Such situations rarely occur, but they do.

Issues are closed when PR is merged to default branch in repo. It should work if you merge this PR directly to master branch.

https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords

@TomasVotruba
Copy link
Contributor

I know what's written there. My point is documentation !== code

@rob006
Copy link

rob006 commented Nov 8, 2019

I know what's written there. My point is documentation !== code

Can you give an example? Happyr/Doctrine-Specification#234 did not closed issue because it was merged do 1.0 branch while default branch for this repo is master. It worked exactly as explained in documentation.

@peter-gribanov
Copy link
Contributor Author

@rob006 Happyr/Doctrine-Specification#234 merged into 1.0 branch and then branch 1.0 merged into master Happyr/Doctrine-Specification#235. That is, the fix PR merged in master, but not immediately.

@peter-gribanov
Copy link
Contributor Author

@rob006 although, perhaps you are right. The problem is that PR did not merge immediately into master and the hook in GitHub did not work. If i meet more such cases, i will describe them here.

@rob006
Copy link

rob006 commented Nov 8, 2019

@peter-gribanov I don't think that you can interpret this as "PR was merged to master". It works in this way for commits, because commits are included in master branch as a part of git tree logic (and AFAIK it should work if commit message would contain correct keyword). But PR is just an abstraction, and merge is an event that occurs once (and has only one target branch). There is no direct relation between different PRs, GitHub does not seem to track this in a way that you may expect in this case.

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.

Functions should do one thing
3 participants