4

I've never rejected a pull request, I just out in loads of comments with constructive criticism.

When shoukd you reject and when should you just sdd in comments?

Comments
  • 4
    Reject everything that isn't on par with the standards, doesn't do what it's supposed to, doesn't actually work or potentially breaks something else (yeah, those PRs happen). My rule of thumb is: if the comments I write are all nitpicking, I don't reject the PR, else I do.
  • 0
    @100110111 From the perspective of the requestor, how would I approach you to talk about the issues in my PR? Just an e-mail to schedule a meeting? Or do you provide a detailed rejection description?
  • 3
    @alexbrooklyn at least in our team you say everything that needs saying in the comments if possible, otherwise just go talk to them (we all sit in the same room when not wfh, so it's pretty easy to talk about these things) or open a convo in slack or something. During wfh times we sometimes scheduled meetings, too. Well... more like: (in slack) "Hey, we need to talk about your PR, meeting now?". I think my next option would be email if none of the above mentioned were possible.

    But that's just how we do things.

    Well, one dev at some point just commented "Fix this shit" to a jrs pr and shouted across the room "You bloody well know what's wrong with it!". That can work, too....
  • 1
    I usually reject PRs I've assigned tasks to. When they finish the tasks they can reopen it. If I didn't do that, people would mindlessly approve everything.
  • 0
    @SortOfTested mindlessly approve? Sounds familiar, can't count the amount of times I added build pipekine requirements with linters
  • 3
    Very different answers depending on if we're talking open source projects or internal teams.

    Internal teams - this almost always comes off the back of a task / story, so it's almost always a case of "comment and improve" rather than reject. Reject in cases where it's so bad that you may as well start again, or the requirements have changed and the story is invalid - but that's rare.

    OS is the other way. Unless it's a clearly useful feature, bugfix, translation update etc. that's been agreed beforehand, chances are it won't be accepted. Particularly relevant in October.
  • 1
    OK. Status fetishist incoming.

    Reject is a poor word choice imho.

    Cause it's just "Nope".

    Reject can be... (Purely fictional - 3 beer and 1 min of thinking)

    "On hold" - Could be merged, but not relevant at the moment (prevention of negatively impacting other PRs)

    "Needs <...>"...
    - Testing: With specific mention what to test (Stress, Benchmark, Integration, ...)
    - QA Approval: Obvious?
    - Polishing: With specific mention what to polish - Coding Style, Commit Style, Newlines, ...
    - Review: In case it was reviewed, would be super duper to know who might know enough about this. Usually it means: Looks good, but someone with more knowledge should take a look at it.
    - Discussion: Anything code related. Best with explanation of who should be involved and why.

    "Not applicable" - Rare reject. Simply for anything that makes the whole PR not applicable to the project - eg. license issues, legal issues, team issues. The PR doesn't fit and will - never - be merged.

    TLDR: A reject should fit in a category. Reject is too broad. Create a workflow and try to make it clear what and why it's not merged (yet). Several, less negative distinctions are possible.
Add Comment