Have a weird question here…So I had a pull request (called PR #X) open that was more or less a “gem install” into a Ruby file. It had 2 code review signoffs by senior developers and was ready to be merged, pending testing on our CI server (which wasn’t working due to a reason beyond our control at the moment). One of the QA folks I work with wanted to get his own PR (PR #Y) in that added an ENV variable that worked with the gem (installing this gem dependent on rspec was a priority of his, though not necessarily the dev team’s). I was going to merge it in but then for some inexplicable reason my tests broke and I couldn’t get them working and had other high priority stuff come up. Also told him we probably had to wait to merge anyway because our CI server wasn’t working.
So then the guy closed my PR without asking me and said they put my changes in his own PR and then asked our slack channel to review his new PR #Z (a combination of X and Y) instead. This all happened within the span of 1.5 days. There was no real urgency in a business sense in terms of getting PR #Z merged, it was just one of those things we wanted to start adding to your workflow (sort of on the order of switching from Sublime to Vim)
My question is - would you have asked the person before you closed their PR (and if you would, is it considered rude not too ask)? And in general, is there any general etiquette around pull requests and the like? In general, I try to close them as fast as possible and if it were me, I would have talked to the person first.
There should always be a person in charge of closing each issue. Starting with project owner… if there are multiple devs who have control of closing issues in a project then the issue/pull-request should be assigned to a person who is responsible for vetting it.
I don’t know how your code stopped working. I recommend CI testing on all submissions before merge. And as for gems like rspec driven that would be a “development” dependency so it can be included as such in the gemspec.
[quote=“danielpclark, post:2, topic:423”]
There should always be a person in charge of closing each issue.
[/quote] - Good point. It helps to prevent miscommunication like this.
As an fyi regarding the broken test - it was easy enough to fix by re-cloning the repo and I’m telling this to give the context. It was probably just an inadvertent typo somewhere that was causing rspec to choke.
I would think that being courteous and letting the other person know of your intentions is good etiquette.
Also why not raise the topic with the other person and let them know that you don’t want to kick up a fuss but you would have liked to have been consulted or notified of their plans as a common courtesy if nothing else.
I think PR etiquette varies from organization to organization.
It seems that you may feel that someone has overstepped their bounds. Maybe the problem here is one of communication. If you feel it was wrong to close your PR, why? Have you spoken to other party about this? Why did they feel it was OK to close your PR in favor of their own?