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.

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

Don't merge your own Pull Requests #70

Closed
nosami opened this issue Feb 10, 2015 · 13 comments
Closed

Don't merge your own Pull Requests #70

nosami opened this issue Feb 10, 2015 · 13 comments

Comments

@nosami
Copy link
Contributor

nosami commented Feb 10, 2015

Ok. I'm guilty of this as well, but from now on, nobody is to merge their own code into master without getting it signed off by one of the other members of the OmniSharp team.

To be merged in, all code must be formatted with spaces not tabs, with 4 spaces wide and using statements ordered correctly.

Also, unless there are exceptional circumstances and you can rationalise them, you must provide tests that show what your code is doing. This is especially important when we are using reflection against a fast moving API. The tests also should serve as documentation for other team members to use.

Anyone in breach of these conditions will be booted off the team :D

@davidfowl
Copy link
Member

Yes!!!!

@Yantrio
Copy link
Member

Yantrio commented Feb 10, 2015

👍

@shanselman
Copy link
Member

Yep.

@jrieken
Copy link
Contributor

jrieken commented Feb 11, 2015

👍

We should start a contributors guide including the formatting rules etc

@mat-mcloughlin
Copy link
Member

Agreed, can I suggest here is a good place to start?

https://github.com/NancyFx/Nancy/blob/master/CONTRIBUTING.md

"Wait for @thecodejunkie to merge your changes in and reformat all of your code because he has StyleCop OCD 😉"

@thecodejunkie
Copy link

Merging your own pull-requests is pointless.. you might as well push straight to master.. and why not force push while you are on it? ;)

Code -> Push a branch -> Send a PR -> Someone else reviews it -> Pulls it if it's OK

Workflow to live by

@mat-mcloughlin
Copy link
Member

Also as a side note this. Is it worth revisiting who is able to merge requests. I'm not wanting to push anybodies nose out but I think there should be a couple of core contributors that can/want to monitor the project and keep it going in the right direction.

Thoughts?

@filipw
Copy link
Member

filipw commented Feb 11, 2015

reminds me of this

@mat-mcloughlin
Copy link
Member

ahahaha

@jchannon
Copy link
Member

Agree. Nancy minions only got write access once it was unmanageable for Andreas & Steve to handle all the PRs even then we have at least 2 minions review before merging.

I'd say Jason should be primary merger and then anyone else he thinks should be allowed to. Others can obviously say I think it looks ok but for now Jason only merges.

Thoughts?

@mat-mcloughlin
Copy link
Member

Can you disable PR ability for project owners?

@jchannon
Copy link
Member

Doubt it. Can we not just make Jason project owner of the server then plugin authors owners of their repo?

@nosami
Copy link
Contributor Author

nosami commented Feb 11, 2015

I don't mind being the owner, but I'm not particularly fussed either way. I think we should just stick to someone else merging for a while and see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants