Skip to content

Update dev docs for GitHub Squash and merge option - #15100

Merged
pllim merged 5 commits into
astropy:mainfrom
taldcroft:docs-squash-and-merge
Aug 10, 2023
Merged

Update dev docs for GitHub Squash and merge option#15100
pllim merged 5 commits into
astropy:mainfrom
taldcroft:docs-squash-and-merge

Conversation

@taldcroft

@taldcroft taldcroft commented Jul 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This pull request is to update the developer docs to describe potential use of the GitHub Squash and merge option when merging PR's. The intent is to enable this option for the astropy repository, reducing effort for contributors and maintainers when squashing is desired.

See https://astropy--15100.org.readthedocs.build/en/15100/

Fix #14473

@taldcroft taldcroft added this to the v6.0 milestone Jul 28, 2023
@taldcroft
taldcroft requested a review from pllim July 28, 2023 19:42
@github-actions

github-actions Bot commented Jul 28, 2023 •
edited by pllim
Loading

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim mentioned this pull request Jul 28, 2023
Closed
nstarman
nstarman reviewed Jul 30, 2023
View reviewed changes

@nstarman nstarman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This all looks great!

eerovaher
eerovaher reviewed Aug 1, 2023
View reviewed changes

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I quite like the new text (although i do have a few small suggestions), but I don't like that the text is split across two different files. Wouldn't it be better to have all the rebasing and squashing instructions in a single location?

Comment thread docs/development/when_to_rebase.rst Outdated
Comment thread docs/development/when_to_rebase.rst Outdated
Comment thread docs/development/when_to_rebase.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
@taldcroft

taldcroft commented Aug 2, 2023

Copy link
Copy Markdown
Member Author

Wouldn't it be better to have all the rebasing and squashing instructions in a single location?

Agreed, done! (just building the new version now).

@taldcroft
taldcroft force-pushed the docs-squash-and-merge branch from 8ae354f to f729f08 Compare August 2, 2023 10:32
@taldcroft

taldcroft commented Aug 2, 2023

Copy link
Copy Markdown
Member Author

Rebased on main with the new pydata theme.

@taldcroft

taldcroft commented Aug 2, 2023

Copy link
Copy Markdown
Member Author

@pllim - Can we enable Squash and merge just before merging this PR? I think the 3 commits should be squashed to one. 😄

@pllim

pllim commented Aug 2, 2023

Copy link
Copy Markdown
Member

Maybe but I won't have time to look at this this week though I am very interested.

@taldcroft
Update dev docs for GitHub Squash and merge option
9854716 
- Apply suggestions from @eerovaher code review
- Merge when_to_rebase into dev workflow
- Fix sphinx warnings and restore accidentally remove content
- Fix another sphinx warning
@taldcroft
taldcroft force-pushed the docs-squash-and-merge branch from 77e1350 to 9854716 Compare August 2, 2023 16:54
eerovaher
eerovaher reviewed Aug 3, 2023
View reviewed changes

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The updated content seems to be good, but I have a handful of formatting suggestions.

Comment thread docs/development/workflow/development_workflow.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
Comment thread docs/development/workflow/development_workflow.rst Outdated
@pllim

pllim commented Aug 3, 2023

Copy link
Copy Markdown
Member

RTD completed successfully this time.

@taldcroft

taldcroft commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

Thanks for the review and suggestions @eerovaher !

nstarman
nstarman approved these changes Aug 7, 2023
View reviewed changes

@nstarman nstarman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

✅. Thanks @taldcroft!

@eerovaher

eerovaher commented Aug 7, 2023

Copy link
Copy Markdown
Member

The commits need to be squashed, but other than that this should be ready.

@pllim

pllim commented Aug 7, 2023

Copy link
Copy Markdown
Member

I think @taldcroft wants to just the "squash and merge". 😉

I won't have time to properly look at this until next week though.

pllim
pllim reviewed Aug 10, 2023
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a few more things to add, looking back at my own notes at #14473. I think it is faster if I push commit to this branch directly. Hope you don't mind.

pllim
pllim reviewed Aug 10, 2023
View reviewed changes
Comment thread docs/development/workflow/development_workflow.rst Outdated
integration tests to run correctly.
* There are merge commits from ``astropy/main`` in the PR commit history (merge
commits from PRs to the user's fork are fine).
* There are commit messages that violate the code of conduct.

@pllim pllim Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rebasing won't get rid of offensive commit messages, so not sure why this was added.

Suggested change
* There are commit messages that violate the code of conduct.

@pllim pllim Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless you also mean interactive rebase to squash?

@pllim pllim Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe I will move this one to the "squash" section instead.

pllim
pllim reviewed Aug 10, 2023
View reviewed changes
Comment thread docs/development/workflow/development_workflow.rst Outdated

.. _github-squash-and-merge:

Github 'Squash and merge'

@pllim pllim Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Moving this section to maintainer workflows page. Also going to add more info to it.

@pllim
Add checkbox to PR template.
599576c 
Move section to maintainer workflow. Clarify policy.
pllim
pllim approved these changes Aug 10, 2023
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added my review as a commit directly. If no objection, let's enable this button!

@pllim

pllim commented Aug 10, 2023

Copy link
Copy Markdown
Member

pre-commit.ci autofix

@pllim
pllim enabled auto-merge (squash) August 10, 2023 20:48
@pllim
pllim merged commit a7f55f5 into astropy:main Aug 10, 2023
@pllim

pllim commented Aug 10, 2023

Copy link
Copy Markdown
Member

I did squash-merge + auto-merge 😸

a7f55f5

Thanks, all!

@pllim pllim mentioned this pull request Aug 10, 2023
Open
@taldcroft

taldcroft commented Aug 11, 2023

Copy link
Copy Markdown
Member Author

Thanks @pllim for getting this across the finish line!

@taldcroft
taldcroft deleted the docs-squash-and-merge branch August 11, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eerovaher eerovaher eerovaher left review comments
@pllim pllim pllim approved these changes
@nstarman nstarman nstarman approved these changes

Assignees

No one assigned

Labels

Docs no-changelog-entry-needed

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

Can we finally enable "Squash and Merge"?

4 participants

@taldcroft @pllim @eerovaher @nstarman