feat: ✨ bring in content on commits and PRs from elsewhere - #157
feat: ✨ bring in content on commits and PRs from elsewhere #157joelostblom wants to merge 16 commits into
Conversation
joelostblom
changed the title
signekb
left a comment
signekb
left a comment
There was a problem hiding this comment.
Very, very nice work! 🌟
This did take quite a long time to review (quite a lot of line changes, resulting in 44 suggestions and comments), so I wonder if it would've been nicer as two separate PRs, one for commits and one for PRs 🤔
github-project-automation
Bot
moved this from In review
to In progress
in Platform development
Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
joelostblom
left a comment
joelostblom
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! I agree that we probably could have split this into two but there was also some content that moved between the files so in that change it was helpful to see it together.
| Avoid stacked PRs, where one open PR targets the branch of another open | ||
| PR, unless there is an exceptional reason to use them. Dependencies | ||
| between PRs make review and merging harder. We prefer independent changes; | ||
| otherwise, clearly identify the dependency and required merge order. |
There was a problem hiding this comment.
We might reconsider this now that the stacked PR feature on github is evlolving, but I left it for now
signekb
left a comment
signekb
left a comment
There was a problem hiding this comment.
Nice work ☀️ A couple of very minor suggestions:
github-project-automation
Bot
moved this from In review
to In progress
in Platform development
Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
joelostblom
left a comment
joelostblom
left a comment
There was a problem hiding this comment.
Thanks, updated! I know your away on vacation so I'll let this sit for a bit and then either ping someone else or self-merge since there were only minor edits since your last approval.
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Very nice! Some suggested changes 🎉
There was a problem hiding this comment.
Could you also delete the text from where you pulled it in those other repos?
There was a problem hiding this comment.
- docs: delete old PR info rostools/git4cats#142 (I cannot add this one to our kanban since it is ina different org, just a heads up)
- refactor: ♻️ move Git and GitHub content into guidebook team#404
| The `<description>` section is a short summary of the change. Write it | ||
| in the imperative, present tense (i.e., "change" not "changed" nor | ||
| "changes") without a period at the end. Be specific enough that another | ||
| person can distinguish the commit from others without reading the diff. |
There was a problem hiding this comment.
| person can distinguish the commit from others without reading the diff. | |
| person can distinguish the commit from others without reading the difference. |
There was a problem hiding this comment.
Here, I would keep diff since it is referring to the specific term. I clarified by prefixing with "commit"
| person can distinguish the commit from others without reading the diff. | |
| person can distinguish the commit from others without reading the commit diff. |
github-project-automation
Bot
moved this from In review
to In progress
in Platform development
joelostblom
left a comment
joelostblom
left a comment
There was a problem hiding this comment.
Thanks, updated and ready for another review!
Co-authored-by: Luke W. Johnston <lwjohnst86@users.noreply.github.com> Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
3 participants
Description
This PR brings together the content from the following sources (let me know if there was some other location with relevant we had in mind):
Closes #148, closes #147
Needs a thorough review.
Checklist
just run-all