Skip to content

Document mergify and issue before PR #126

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 20, 2020
Merged

Document mergify and issue before PR #126

merged 2 commits into from
Aug 20, 2020

Conversation

ravwojdyla
Copy link
Collaborator

Add mergify and "issue before PR" in contributing docs.

Re: #125 and #107

@ravwojdyla ravwojdyla added the auto-merge Auto merge label for mergify test flight label Aug 20, 2020
@ravwojdyla ravwojdyla requested review from tomwhite and hammer August 20, 2020 12:00
jeromekelleher
jeromekelleher previously approved these changes Aug 20, 2020
View reviewed changes
Copy link
Collaborator

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM

docs/contributing.rst Outdated
@@ -257,5 +257,12 @@ without warnings. You can do that locally with::
Review process
--------------

Pull requests will be reviewed by a project maintainer. All changes to *sgkit* require
approval by at least one maintainer.
All pull requests require pre-existing issue. If you plan to submit a PR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
All pull requests require pre-existing issue. If you plan to submit a PR,
All pull requests require a pre-existing issue. If you plan to submit a PR,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed - thanks!

jeromekelleher
jeromekelleher reviewed Aug 20, 2020
View reviewed changes
@jeromekelleher jeromekelleher self-requested a review August 20, 2020 12:57
@jeromekelleher jeromekelleher dismissed their stale review August 20, 2020 12:58

Changed my mind over the approval and don't want it automerged.

jeromekelleher
jeromekelleher approved these changes Aug 20, 2020
View reviewed changes
Copy link
Collaborator

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Thanks @ravwojdyla, LGTM!

@ravwojdyla
Copy link
Collaborator Author

FYI, and it looks like we still need to configure the mergify installation on pystatgen to include this repo.

tomwhite
tomwhite approved these changes Aug 20, 2020
View reviewed changes
docs/contributing.rst Outdated
Pull requests will be reviewed by a project maintainer. All changes to *sgkit* require
approval by at least one maintainer.
All pull requests require a pre-existing issue. If you plan to submit a PR,
please create an issue first to describe and discuss your idea/plan/problem.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add the bit about mentioning the issue in the PR comments here from https://github.com/pystatgen/sgkit/issues/125

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@ravwojdyla ravwojdyla mentioned this pull request Aug 20, 2020
Merged
@mergify mergify bot merged commit 60c9ab2 into master Aug 20, 2020
@ravwojdyla
Copy link
Collaborator Author

ravwojdyla commented Aug 20, 2020 •
edited
Loading

Ok - so that (auto merge + branch deletion) worked fine.

@mergify mergify bot deleted the rav/doc_mergify branch August 20, 2020 16:31
@jeromekelleher
Copy link
Collaborator

Ooh, why did mergify merge master in here? Wouldn't it be better to rebase onto to the base branch? I think we've got things the wrong way around - we should rebase to bring the branch up to date and merge to merge it in. That way the network graph is much more comprehensible:

Screenshot from 2020-08-20 17-33-28

vs

Screenshot from 2020-08-20 17-35-11

Maybe I should make this an issue.

@ravwojdyla
Copy link
Collaborator Author

ravwojdyla commented Aug 20, 2020 •
edited
Loading

@jeromekelleher actually, doing it this way is:

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

@tomwhite tomwhite tomwhite approved these changes

@jeromekelleher jeromekelleher jeromekelleher approved these changes

@hammer hammer Awaiting requested review from hammer

Assignees
No one assigned
Labels
auto-merge Auto merge label for mergify test flight
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ravwojdyla @jeromekelleher @tomwhite