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

add hook for workflow #17

Merged
merged 6 commits into from
Aug 1, 2020
Merged

add hook for workflow #17

merged 6 commits into from
Aug 1, 2020

Conversation

zhouronghua
Copy link
Contributor

need to hook for workflow

@zhouronghua zhouronghua requested review from a team and bwoodsend and removed request for a team July 30, 2020 05:02
delete #-----
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

This looks fine, but you need to add a changelog file. See the news readme.

@Legorooj Legorooj merged commit 34e8355 into pyinstaller:master Aug 1, 2020
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

Almost. This is PR 17 so you just need to rename that file.

@bwoodsend
Copy link
Member

Ahh I see this is already merged. @Legorooj Would it be easier to revert and ask zhouronghua to correct the name or just sort it ourselves?

@Legorooj
Copy link
Member

Legorooj commented Aug 1, 2020

@bwoodsend I didn't notice that, so good catch, but the number is actually semi-valid: #19. There's probably no point changing it now, cause that issue links back to this PR (albeit in a roundabout manner).

In this case, amending the commit locally and pushing is probably the easiest thing - I enabled force pushing to this repository. When you get write access to the main PyInstaller repository remember you can't force push, so no history rewriting. Check 5 times more than necesarry before merging something on the main repo.

@zhouronghua
Copy link
Contributor Author

Ahh I see this is already merged. @Legorooj Would it be easier to revert and ask zhouronghua to correct the name or just sort it ourselves?

sorry, not familier with the workflow, i thought the number was the issue i just generate, but in fact it is the pull request no

@Legorooj
Copy link
Member

Legorooj commented Aug 1, 2020

@zhouronghua issues and PRs share the numbers, so if there's no issue for it already, then just use the PR number.

@zhouronghua
Copy link
Contributor Author

@zhouronghua issues and PRs share the numbers, so if there's no issue for it already, then just use the PR number.
i get it, thank u

bwoodsend pushed a commit that referenced this pull request Aug 1, 2020
@bwoodsend
Copy link
Member

There we go. Sorted now. @zhouronghua For future reference - the xxx.new.rst file goes in the news/ folder rather than the root. Other than that - good work!

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.

3 participants