krestenkrab / triq

Trifork QuickCheck
http://krestenkrab.github.com/triq/
Apache License 2.0
281 stars 54 forks source link

Handling of Pull Requests #47

Open zkessin opened 9 years ago

zkessin commented 9 years ago

This project has a few Pull Reqs that are quite old. I would like to suggest a rule that Pull Reqs should be dealt with quickly, be they accepted or rejected, but they should not be left to hang around.

Comments please

MirkoBonadei commented 9 years ago

Hi @zkessin, I agree with your suggestion. I am interested in this project even if I am out of the game in this period. I think that it is not easy for @krestenkrab to approve pull requests right away because this project is not in his priorities and mind context switch among projects is not easy at all. @krestenkrab what can we do to help you in the approval phase?

zkessin commented 9 years ago

@krestenkrab would you be interested in having a few of us help? the zeromq community has a good set of rules that can be adopted for how to grow triq http://rfc.zeromq.org/spec:22 (though I think leaving the license as is would be good)

ghost commented 9 years ago

Reasonable suggestion if a proper dev process (review and branching model) is used, in order to avoid a chaotic or unstable master branch.

zkessin commented 9 years ago

take a look at what I posted for a model, the 0MQ folks have done very well with that model.

essen commented 9 years ago

That's a very heavy model for fixing responsiveness issues. Also I think someone summed it up as "merge first, ask questions later"? Probably referring to Maintainers SHALL NOT make value judgments on correct patches.

A much better model would be the original git model. Kresten could get one or more trusted contributor to handle the PRs, merge them in their own fork and then submit the update to this very repository. At this point Kresten just has to take a quick glance and click merge in the large majority of the cases.

hintjens commented 9 years ago

Changing culture can be heavy, yes. However we've found that merging rapidly gives new contributors the incentive to learn and stay. It's worked very well in dozens of projects now. The only drawback seems to be some vulnerability to trolls, though this is easily managed, it turns out.

If you're seeing PRs sitting unmerged, due to lack of time to review them, I'd definitely check out C4, and ask the zeromq-dev list (mostly users of ZeroMQ, so critical of any faults in the process) how they like the process.

zkessin commented 9 years ago

The real issue is that I would like to add some enhancements to Triq, but am not going to do so unless I have some confidence that Pull Reqs will actually get merged

essen commented 9 years ago

Hello @zkessin, I saw your invite and am wondering if you could explain a little more about it, like whether you talked about it with Kresten and what are your plans for it? Thanks.

krestenkrab commented 9 years ago

Folks, I'm somewhat hung up .. so if one of you cool erlangers want to co-administer this you're most welcome.

zkessin commented 9 years ago

I want to manage the open-triq branch under the zeromq rules, I have invited @essen @krestenkrab @MirkoBonadei and a few others to the project as committers, (and will invite anyone else after a few merged Pull Reqs)

I hope this way Triq can move forward and @krestenkrab does not need to be involved if he does not have time

krestenkrab commented 9 years ago

I hope we can keep triq under Apache license.

zkessin commented 9 years ago

It will stay under the same licence

zkessin commented 9 years ago

Readme changed to make the licence clear

ghost commented 9 years ago

Given the size of Triq, a dual branch model is sufficient to allow:

Also due to Triq's size there's no need for collecting topic branches into a next branch and merge only stable topics to master.

So, if we change the project settings to merge pull requests to a staging branch (with immutable history), we can have co-administered merges of patches.

Kresten has offered to assign permissions as required, so I don't see a need for a separate project.

hintjens commented 9 years ago

The use of a single master branch for all work is aggressive, yet it has proven successful in multiple projects now, of all sizes and maturities. Merging to master forces errors into public view sooner, and puts the economic onus on contributors, rather than maintainers. It has reduced the error count in our projects dramatically, and made it much easier for new participants to join.

On Mon, Mar 9, 2015 at 4:56 PM, Tuncer Ayaz notifications@github.com wrote:

Given the size of Triq, a dual branch model is sufficient to allow:

  • quick merges of good patches
  • keeping a stable master
  • merging to master on schedule, assuming it's in a regression-free state

Also due to Triq's size there's no need for collecting topic branches into next and merge only stable topics.

So, if we change the project settings to merge pull requests to a staging branch (with immutable history), we can have co-administered merges of patches.

Kresten has offered to assign permissions as required, so I don't see a need for a separate project.

— Reply to this email directly or view it on GitHub https://github.com/krestenkrab/triq/issues/47#issuecomment-77881331.

essen commented 9 years ago

I would agree with that. I do not see much point in a branch or even less a fork. This project just needs someone with time to review and merge changes.

zkessin commented 9 years ago

Well ideally what it needs is a few people who can collectively deal with pull requests. I don't want a bunch of pull requests to pile up because one person goes on holiday or gets swamped at work etc

zkessin commented 9 years ago

Just wait for Travis - CI to run the tests before you merge a pull request

hintjens commented 9 years ago

We had a few significant breakthroughs in the ZeroMQ process. One was to split off reviewing PRs from merging them. We simply do not do blocking reviews. This sounds insane yet produces better code. To merge a PR takes just a quick sanity check, and even that is redundant. If someone breaks master, that is significant and useful data. And fixing the break is good learning, either for the contributor, or for others.

This only works if you (a) use a single branch, (b) promote contributors aggressively to maintainer and (c) have tools for regulating bad actors (thus a well written contract of sorts).

On Mon, Mar 9, 2015 at 5:19 PM, Zachary Kessin notifications@github.com wrote:

Well ideally what it needs is a few people who can collectively deal with pull requests. I don't want a bunch of pull requests to pile up because one person goes on holiday or gets swamped at work etc

— Reply to this email directly or view it on GitHub https://github.com/krestenkrab/triq/issues/47#issuecomment-77886476.

zkessin commented 9 years ago

I also like the idea of having it under an organizational ownership, not @krestenkrab 's personal account

essen commented 9 years ago

This project barely has any tests, relying on Travis or similar is suicidal at best.

zkessin commented 9 years ago

well we can add more, but it might as well pass the tests that it has

ghost commented 9 years ago

@hintjens, how do you avoid introduction and subsequent fixing of bad APIs, which means more maintenance overhead due to extra deprecations and compat code, without code reviews?

hintjens commented 9 years ago

With libzmq we also had few test cases. We enabled Travis, and said, "breaking existing test cases is a reason to revert (or not merge) a patch", and with this incentive, new contributors added a lot of test cases. This was very positive.

However I don't usually wait for Travis before merging. Like I said, breaking master is good data.

On Mon, Mar 9, 2015 at 5:28 PM, Zachary Kessin notifications@github.com wrote:

well we can add more, but it might as well pass the tests that it has

— Reply to this email directly or view it on GitHub https://github.com/krestenkrab/triq/issues/47#issuecomment-77888631.

hintjens commented 9 years ago

@tuncer here's my latest proposal for that: http://hintjens.com/blog:85

Basically you allow APIs to evolve naturally, and you tag them RAW, DRAFT, STABLE, LEGACY, RETIRED depending on use. This gives you a tool for fixing design errors (during RAW and DRAFT), while not breaking user space (STABLE onwards).

The underlying change theory is, deliver code rapidly to real people, and allow them to express irritation in the form of pull requests rather than issues. Done right, you get a smooth and fast cycle of change that iterates through refinements until APIs and internals are good, and stable, and the result sits for a year or two before being superseded by a better abstraction. You then make a new draft, and start to deprecate the old APIs (e.g. remove from documentation). The library can support them for long, the cost is usually low.

zkessin commented 9 years ago

anyone know how to get travis-ci to work with a new organization? It does not turn up in my travis-ci screen

hintjens commented 9 years ago

You need to sign into Travis with the credentials of that organization, I think.

On Mon, Mar 9, 2015 at 5:46 PM, Zachary Kessin notifications@github.com wrote:

anyone know how to get travis-ci to work with a new organization? It does not turn up in my travis-ci screen

— Reply to this email directly or view it on GitHub https://github.com/krestenkrab/triq/issues/47#issuecomment-77892544.

zkessin commented 9 years ago

Ok, so who here likes the idea of using the ZeroMQ Model, please leave a +1 comment so we can get a sense.

Obviously if there are any questions then ask them

ghost commented 9 years ago

@hintjens, how do you convey what stage an Erlang function is at to a random consumer of the API?

essen commented 9 years ago

I would guess as part of the documentation.

Anyway I'm not too interested in a process that breaks master all the time, I like me git bisect.

ghost commented 9 years ago

@zkessin, I can understand why a separate org is preferred, but:

As krestenkrab/triq is under a personal (not company) profile, I don't mind keeping the canonical url unchanged, but I'm okay with a separate org that has a better name :). It has to be worth it, though, as it will take a very long time for users to realize the new url, to mention one problem.

zkessin commented 9 years ago

the problem was that the name triq was not available, if you have a better name for the org i would be happy to take suggestions

ghost commented 9 years ago

I totally understand the benefits of the proposed model and can relate to the issues the other models impose on everybody, but merging everything that doesn't look totally broken to master doesn't appeal to me in the proposed form. First of all, you need enough qualified contributors on call to unbreak the branch in a matter of hours max. That means, a project short of contributors suddenly is in need of more contributors. Second of all, doing no review of interface changes means more often than not the maintainers will have to do the cleanup or refactoring rather than the patch author, who may never reappear, and the maintainers may have to guess and fill in the blanks, or not have access to certain operating environments. Finally, in the merge-quickly scenario it becomes more important that CI includes the project being tested against as many existing users (projects) of it as possible, which usually requires a nightly run.

So, I think there has to be a healthy middle-ground between merge-anything and pending-for-review-since-last-year.

ghost commented 9 years ago

@zkessin if a separate org is the way to go, I'm sure we'll find a better name. Let's ignore the name for now.

zkessin commented 9 years ago

ok, we will leave it as @open-triq if we think of a better one later we can change it

hintjens commented 9 years ago

I'll throw in a few more comments:

Anyhow, this ain't a sale pitch, just material for thought. :)

jtmoulia commented 9 years ago

Hey all, is the issues tracker meant to be closed for open-triq/triq?

zkessin commented 9 years ago

No, I will fix that

ghost commented 9 years ago

@krestenkrab, what's the plan?

ghost commented 9 years ago

@krestenkrab ping?