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.

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

Update PULL_REQUEST_TEMPLATE.md #1505

Merged
merged 5 commits into from
Jul 20, 2018
Merged

Update PULL_REQUEST_TEMPLATE.md #1505

merged 5 commits into from
Jul 20, 2018

Conversation

luceos
Copy link
Member

@luceos luceos commented Jul 13, 2018

updates our PR template to clarify a few things

fun thing having to fill out the PR template when updating the PR template..

Fixes nothing

Changes proposed in this pull request:

  • PR template

Reviewers should focus on:

  • required steps before sending in a PR
  • the comment description

@luceos
Update PULL_REQUEST_TEMPLATE.md
365020f 
updates our PR template to clarify a few things
franzliedke
franzliedke previously requested changes Jul 13, 2018
View reviewed changes
.github/PULL_REQUEST_TEMPLATE.md Outdated
**Confirmed**

- [ ] for visual or frontend changes that the changes work as intended via visual confirmation on a local flarum installation.
- [ ] for php changes that the unit tests are all AOK (green) by running them locally (`php vendor/bin/phpunit`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay for the checklist!

Can we make these a bit shorter, though?

  • frontend changes: tested on a local Flarum installation
  • backend changes: tests are green (run php vendor/bin/phpunit)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

franzliedke
franzliedke reviewed Jul 13, 2018
View reviewed changes
.github/PULL_REQUEST_TEMPLATE.md Outdated
- [ ] for visual or frontend changes that the changes work as intended via visual confirmation on a local flarum installation.
- [ ] for php changes that the unit tests are all AOK (green) by running them locally (`php vendor/bin/phpunit`).

**Required changes:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these meant for suggested follow-ups (e.g. docs) or for dependent, but existing pull requests in other repos (e.g. extensions)? It wasn't clear to me...

Copy link
Member Author

Choose a reason for hiding this comment

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

line 21 is, the lines above that are meant as checks/requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is:

  • "Docs" sounds like more of a TODO for later,
  • "Core extensions" seems to be the place for links to related PRs.

Or should both contain links to related PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally docs should be PR'd at the same time (at least once we have a good foundation of docs!)

Copy link
Contributor

Choose a reason for hiding this comment

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

So can we just combine these two with the list above?

  • Related documentation PR: (Remove if irrelevant)
  • Related extension PRs: (Remove if irrelevant)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice one.

@luceos
Copy link
Member Author

luceos commented Jul 16, 2018

@franzliedke i leave the final review and merge to you. I've updated the top description a little. We can iterate later if we feel it could use more love. The suggestions were integrated.

@tobyzerner
Copy link
Contributor

Can we also add "updated changelog" as a checklist item. From now on I'd like to keep a CHANGELOG.md in the root.

@luceos
Copy link
Member Author

luceos commented Jul 16, 2018

@tobscure good point.

@flarum/core with these kind of PRs, just use the inline github edit to add your suggestions (if they seem obvious)?

@franzliedke
Copy link
Contributor

Regarding the changelog: This is just asking for lots of conflicts between PRs, especially if they sit around for a while (ahem).

@luceos
Copy link
Member Author

luceos commented Jul 16, 2018 •
edited
Loading

Not if they are added to a heading called # next? We can create such a thing per default.

Or, instead of pointing to a changelog, we can point to a different "hoarder" issue for the next release?

@franzliedke
Copy link
Contributor

@luceos But everyone will add their change at the bottom of that file, meaning we will have multiple inserts. Still a conflict. 😕

@tobyzerner
Copy link
Contributor

True. Relevant discussion:

isaacs/github#487
olivierlacan/keep-a-changelog#56

@luceos
Copy link
Member Author

luceos commented Jul 17, 2018

We can still pull the changes from the PR description once we do a release. Doing a release has been a manual action so far anyway.

@luceos luceos dismissed franzliedke’s stale review July 17, 2018 05:55

no longer valid

@tobyzerner
Copy link
Contributor

I would really like to keep a running changelog. Having to go through issues, PRs, and commits manually (as I have just done for beta 8) is extremely tedious.

@luceos
Copy link
Member Author

luceos commented Jul 17, 2018

@tobscure how about a release issue for the upcoming release named Changelog?

@franzliedke
Copy link
Contributor

franzliedke commented Jul 17, 2018 •
edited
Loading

What is keeping us from using the release milestone then? "Some changes are not related to issues"?

What about a GitHub project per release then, where we can add any small change that we want to keep track of as one of these low-fidelity "notes"? That way, we don't have to worry about the order of changes (and thus, conflicts), but still have all the changes that we want to track in one place.

@franzliedke
Copy link
Contributor

Also fun, but probably overdoing it a bit:
https://about.gitlab.com/2018/07/03/solving-gitlabs-changelog-conflict-crisis/

😅

@tobyzerner
Copy link
Contributor

I suppose we could probably get away by reviewing milestones if we make sure every significant change (worthy of mention in the release notes) is done as a PR or has an issue associated with it?

But yeah there's no good solution for CHANGELOG.md merges so let's leave that off the checklist.

@dsevillamartin
Copy link
Member

This is why having a Commit template such as angular’s can be nice when creating a CHANGELOG automatically or manually.

@luceos
Copy link
Member Author

luceos commented Jul 18, 2018

@datitisev link? example?

@dsevillamartin
Copy link
Member

Angular CONTRIBUTING.md: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md
Angular Commit Guidelines: https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#commits

Then their changelog is generated with cz-conventional-changelog, I think (uses Commitizen to make writing commits easier)
image

franzliedke
franzliedke approved these changes Jul 19, 2018
View reviewed changes
Copy link
Contributor

@franzliedke franzliedke left a comment

Choose a reason for hiding this comment

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

I removed the changelog part for now. Feel free to merge if you think it's okay.

I think it's great. 👍

@tobyzerner tobyzerner merged commit f89c111 into master Jul 20, 2018
@tobyzerner tobyzerner deleted the luceos-patch-1 branch July 20, 2018 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobyzerner tobyzerner tobyzerner left review comments

@franzliedke franzliedke franzliedke approved these changes

Assignees

@franzliedke franzliedke

Labels
meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luceos @tobyzerner @franzliedke @dsevillamartin