On pull requests

[Alex Ruiz]

Hey team,

Please let's not accept any pull requests for now. I know it doesn't sound right but please let's do it, as a favor to me. I'm busy understanding the code base now and the more it changes the longer it will take me.

BTW, I'm going to start reverting some pull requests that I feel really uncomfortable with. The reasons are either quality of the feature (e.g. is this really necessary?), code quality or failing tests (e.g. tests for Files.contentOf(File) fail on Mac, so bye bye.) The problem is that is not easy to understand some of the pull requests (sorry, some are too messy) and it will take time.

I'd rather have a small, clean API for FEST-Assert 2.0 than one that includes everything and the kitchen sink. Once we give away something we cannot take it back, unless we have FEST-Assert 3.0, which will be a PITA.

Some external contributors may not like it, but hey! we cannot please everybody. I rather release something solid and have happy users instead of releasing something so-so and have happy contributors.

I was thinking that we rather receive patches than pull requests. Pull requests tend to be messy: they have a good number of commits, and some of them include rollbacks of errors created by the authors...it is quite hard to read. A patch is simpler.

From now on we are going to have at least 2 team members vote on a pull request before accepting it. I think we have too many of "oh it would be nice to have this in the project because I need it" pull requests. We add something new only if the majority of users will benefit from it. If the pull request has to stay there waiting for approval, so be it. We rather be extra, super careful before adding new stuff.

Cheers,

-Alex

[Joel Costigliola]

Alex,

I'm asking you not to revert pull requests before we have a chance to discuss them, see why you are uncomfortable with and come to an agreement wether to keep them or not, then if we can't agree then you will have to decide what is best.

For example, I think Files.contentOf(File) is a valuable addition to make assertions on the content of files, I would prefer we fix the unit tests than removing the feature.

My point of view here is : first let's evaluate how interesting a feature is, then if we like it but the code is messy or not good enough, then we just fix/improve it.

As matter of fact, I take responsibility on accepting maybe too much pull requests, One I'm not too proud includes : areNotAtLeast, areNotAtMost, doNotHaveAtMost, areNotExactly, doNotHaveExactly which makes me uncomfortable. At the time I accepted the contribution because I liked areExactly, areAtMost, ... so I also accepted their negation.

In more general point of view, my philosophy is that Fest should be built with users feedback.
That explains why, when a user asks for a new assertion, I only try to find good reason not to add it (can it be done with existing assertions ? is it too specific ?) and if I don't find any, then I accept it. For me contributors are just Fest users that like Fest but think it could be richer.

So here comes the difficult question : what are the criteria to evaluate new assertions ?
IMHO, The most important criterion is : is it useful ?
And then can we find a good name for the assertion? For me, qualities of good names are  : expressive, not ambiguous, fluent 

I really care about having a rich and easy to use assertions API which meets our user needs, that's why in the first place I believe in this project.
I agree to be merciless on code quality but we should not be merciless on user requests.

Cheers,

Joel

ps : on pull requests vs patch, I usually fetch them in a separate local branch which it is easy to compare with the master branch.
pps : on freezing new pull requests : I'm ok but I hope it won't be on a too long period because I think Fest would suffer from the lack of activity/releases.

On Sun, Aug 26, 2012 at 12:08 AM, Alex Ruiz <alex.r...@gmail.com> wrote:

contentOf

[Olivier Michallat]

Hi Alex,

I contributed Files.contentOf(File). I also work on a Mac, and the tests run fine for me. If you want to investigate further on this issue at some point, don't hesitate to contact me.

Regarding messy pull requests, you could require contributors to rewrite their history (or even squash all their commits into one) and re-submit with 'git push -f'. Normally this is a big no-no, but GitHub seems to handle it properly for pull requests (see [1], [2]).

Personally I won't take it badly if you revert my stuff, do what you think is right for the project.

-- Olivier

[1] http://blog.springsource.org/2011/07/18/social-coding-pull-requests-what-to-do-when-things-get-complicated/
[2] http://davidwalsh.name/squash-commits-git

[Alex Ruiz]

Hey Joel,

I'm on vacation, typing on my iPhone, which is pretty annoying. Sorry for the short e-mail.

I agree that we should build FEST based on user feedback. But we must be extremely careful about what we add because we cannot take them back.

Like I mentioned before, I'll be filing bugs when I consider something needs to be cleaned up. We can discuss the bugs.

I don't think there is a hard rule about pull requests. We evaluate them individually. I wish we had code reviews  using Gerrit like Eclipse does. It makes it very easy to provide feedback to contributors.

Cheers,

Alex

Sent from my iPhone

--
You received this message because you are subscribed to the Google Groups "easytesting-dev" group.
To post to this group, send email to easytes...@googlegroups.com.
To unsubscribe from this group, send email to easytesting-d...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.