Then you (or someone) would give a 
review, no? I mean, we didn’t abandon Gerrit reviews in that case, neither.
… or something like 
or 
️.
Regarding milestones: that would basically be “fix version”, right?
Actually we could decide to just close the pull request and reopen one at a later stage. Your fork with your changes is still there and you can continue to work on it…
In the Symfony Project, pull requests are required to have the following table as part of the PR message:
| Q | A
| ------------- | ---
| Bug fix? | [yes|no]
| New feature? | [yes|no]
| BC breaks? | [yes|no]
| Deprecations? | [yes|no]
| Tests pass? | [yes|no]
| Fixed tickets | [comma separated list of tickets fixed by the PR]
| License | MIT
| Doc PR | [The reference to the documentation PR if any]
We could introduce a variation of that to our own rules.
They also use labels a lot, for example:
Status: Needs Review
Status: Needs Work
Status: Reviewed
Ready
RFC
Bug
Enhancement
Feature
Easy Pick
… and labels for various components.
I could imagine that something like the labels above makes sense for us as well. Maybe we can even synchronise labels somehow with Jira, so that if an issue is mentioned in the PR (which should be the case usually), some script sets labels according to what we have in Jira.
1 Like
I like the labels, I dislike the table requirement.
1 Like
regarding the table:
I don’t like that it’s a table, but we do need the information (and we required it with our Gerrit process already). They question is where to put the information.
“Tests pass” should be determined automatically, license should be clear (GPL for Neos, MIT for Flow), documentation should be contained in the PR.
“Bug fix” / “new feature” / “BC breaks” was communicated in the commit subject, question is if we continue to do that. We could modify it a little and write subjects like:
Bugfix: Make infinite loop less endless
or
(!) Feature: Use PHPLIB instead of Symfony Components
Right?
Sure. I was just thinking about how we did it “back then”. OTOH, it might be a chance to begin using “abandon” more often, which we used only seldomly.
Regarding “WIP” tag/flag - there is still some case where you have some idea on how to implement a specific feature/task and want to show it early to get feedback, where it should be clear that this is not merge-ready. Should those cases then be handled with simply linking to the feature branch in one’s own fork, or would it still be valid to already create a PR?
By now we have a WIP label in github. So I guess PR and adding that should be fine.
We should write down a (recommended) flow for doing code reviews, I guess. It should include at least
- start with a conceptual review (unless it’s a trivia bugfix, of course)
- nitpicks can be done, but if there is a fundamental issue with the PR, sort that out first!
- if possible, fix nitpicks yourself, instead of just commenting on them
- if needed, ask the author for push access to the fork, so you can fix them easily
- or do a PR on the PR (cumbersome, but works)
- add labels and keep them updated
- if a PR has the “discussion” label, check and try to participate
- even if you don’t know the code area affected, your feedback on concepts, new API, documentation, configuration can be very helpful!
- if you assign a WIP label, be prepared that noone reviews your code - if it’s ready for review, don’t mark as WIP
1 Like
good idea, could you add it to Committers Guide? if that doesn’t seem fitting, add a new section to Code contribution guideline
I would like to add the third option for nitpicks to just merge the original change and push a follow up fixing the nits afterwards (which then could probably be merged as no-brainer).
- if you assign a WIP label, be prepared that noone reviews your code - if it’s ready for review, don’t mark as WIP
This is still a no go for me. IMHO everyone should be curious about conceptual ideas and very early feedback is sometimes very helpful to get stuff out of WIP. I usually open PRs as WIP to get feedback…
I would rather come to the formula:
- If you don’t want your code/change reviewed don’t start a pull request (yet). If it is really only halfway done code wait until the concept becomes clear to open a PR.
Same goes for failing style/tests. Both may block a merge, but the concept might be OK just that the code changes break a lot of tests, also there it would be great to get some early feedback on the change itself.
2 Likes
But that should be marked as Feedback, not WIP, no?
Hey together,
I’d say:
- “discussion” means one wants to get feedback on this
- “WIP” means “Hey, I am working on this currently – not yet finished.” Changes marked as WIP should never be merged
Of course to me, both tags can be applied for PR. That’s what I’d suggest if you want feedback on WIP stuff @christianm.
All the best,
Sebastian
1 Like
Would work for me.
So far I have seen the “needs feedback” tag only for expecting feedback from the creator of the change, but “discussion” could work if we agree on this.
Yeah, I also understood “Needs Feedback” as the creator needs to give feedback. I’m also fine with using “Discussion” and “WIP” as Sebastian suggested.
2 Likes
Reading Creating a pull request the labels should be assigned by the creator of a PR, so “Needs feedback” being directed at the creator is weird IMHO. Also, any activity on the PR should be read and acted upon by the creator (why would one open a PR if she’s no longer interested in it?), so simply asking a question should be enough to trigger feedback. I didn’t even realize we have both labels. 
And “Discussion” is described as “PR is blocked by discussion” in that post, so it would be something else as well.
Ok, I just learned that labels can only be assigned by people having write access to a repository. So in a lot of cases that would not be possible by the author of a PR.