On Mon, Sep 26, 2016 at 12:40 PM, Simon Marlow <[email protected]> wrote: > On 26 September 2016 at 20:13, Ben Gamari <[email protected]> wrote: >> >> Simon Marlow <[email protected]> writes: >> >> > I would rather we *didn't* accept contributions via github, even for >> > small >> > patches, and instead put more effort into streamlining the Phabricator >> > workflow. >> > >> > >> > - Adding another input method complicates the workflow, users have to >> > decide which one to use >> > >> I think we would want to try to sell the GitHub route as "if you would >> like to contribute then we would strongly prefer you use Phabricator, >> but if you must and it's a small patch, we will accept it via GitHub." > > > But this is opening the floodgates a crack... how do we know what a "small" > patch is? What happens when someone submits a patch that's too large? The > patches will get larger, we'll have to do code reviews on two different > tools, and it will be really hard to go back. I just have a bad feeling > about this.
I agree that this would certainly happen - people would get used to the new workflow and want to use it for large patches. I've said similar things in a post I made to Carter's thread about a ghc-simple-patch-propose list. Please consider this approach: If a patch requires substantial revision, move it to Phabricator. How do we know when it requires substantial revision? There are two conditions for this: 1) When it is clear to the reviewer that it will take multiple iterations 2) After a single iteration of review has occurred on GitHub, and there is still more to do. This way, any substantial review process will be captured as a differential. We need a way to make this as easy and efficient as possible for maintainers and contributors alike. In particular, maintainer / reviewer time is very valuable. As such, I propose the following: 1) A tool called "ghc-hub", which supports "ghc-hub migrate URL", to migrate a PR to a Differential, without migrating any review metadata (to keep the implementation simple). It would also support "ghc-hub merge URL", . Using PR URLs on the commandline like this is inspired by github's hub tool - https://github.com/github/hub . 2) Eventually, have a GitHub bot which does the following: * Notices if a patch is particularly large, and recommends using the "ghc-hub" tool to perform the migration. * Notices if a patch has already gone through a review and hasn't been merged for a while, and automatically migrates it to a differential. Note that having these tools ready is not necessary to begin this new workflow, but I think they will be helpful for making this new workflow a sustainable approach. If you guys are on board with this approach, I would enjoy writing the initial version of "ghc-hub". Though I cannot at this time guarantee that I would be willing to entirely champion / maintain it - I'll need some help. >> > - Github is not integrated with our other infrastructure, while >> > Phabricator is >> > >> True, but I suspect for the small documentation patches that we are >> currently consider this shouldn't matter so much. >> >> > - Mutliple sources of contributions makes life harder for maintainers >> > >> It does certainly put yet another task on our plates, but I would argue >> that it's actually easier than accepting patches via Trac, which we >> already do. > > > We should stop accepting patches via Trac too :) > > Cheers > Simon > > > >> >> > - I also like the idea of auto-push if validate succeeds. Or a >> > button >> > that you can press on the diff that would do the same thing, so you >> > can get >> > code review first. >> > >> To be clear, I'm a bit weary of opening up the auto-push feature to new >> contributors. While regular contributors know what changes can be safely >> pushed and which require review, we have no guarantee that a new >> contributor has developed these sensibilities. >> >> > - +1 to making the manual easier to build. The same goes for >> > Haddocks; >> > it's really hard to make a simple patch to the docs and test it right >> > now. >> > >> The users guide should be quite possible to do. >> >> I don't believe there is any reliable way to allow a contributor to >> build the haddocks without having built GHC (since you need GHC master to >> parse `base`, et al.); that being said, we could have Harbormaster >> upload built documentation somewhere and then leave a link to it on the >> Diff. >> >> > One other thing that came up but wasn't mentioned in the notes: let's be >> > more prompt about reverting patches that break validate, even if they >> > only >> > break a test. Now that we have better CI support, we can easily >> > identify >> > breaking patches and revert them. >> > >> Agreed. >> >> Cheers, >> >> - Ben >> > > > _______________________________________________ > ghc-devs mailing list > [email protected] > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs > _______________________________________________ ghc-devs mailing list [email protected] http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
