Make chaining available in Stargazers

[Swader]

Allows for:

$stargazers = $client->repo()->stargazers()->configure('star')->all('vuejs', 'vue');

instead of

$stargazers = $client->repo()->stargazers();
$stargazers->configure('star');
$stargazers = $stargazers->all('vuejs', 'vue');

[Nyholm]

Do we really want a fluid interface here?

There are at least 6 other configure functions that also needs to be updated if we want this change.

[Swader]

What are the anti arguments, apart from other configure functions needing an update?

[Nyholm]

Sorry for my empty post. Why fluid interfaces are bad is explained by Ocramius here: https://ocramius.github.io/blog/fluent-interfaces-are-evil/

It is also a matter of opinion. Im not sure if we should introduce it in this library or not. I will let others decide if we want fluid interfaces at all.

[Swader]

I appreciate that post, but do not agree with it. Regardless, I since there doesn't seem to be any interest from the public IRT this topic, you can close this I guess.

[acrobat]

I'm 👎 on this as it is not a bug just another way of calling the methods.

[sagikazarmark]

Although I am also against fluent interfaces, I would probably make an exception in this case and I have two reasons for that:

• there IS such thing as developer experience. In some cases sanity and well-designed API might not allow us to also make the developers happy, but in this case I don't see any reasons why it should not be fluent, on the other hand, it would obviously increase DX
• I am not that familiar with the Github API client, but it seems to me that introducing immutability would be a good idea here, which would automatically make the interface fluent (that's also probably backwards compatible)

[acrobat]

DX is indeed a good point. We could allow fluent setter on the configure methods (other api methods make no sense to do this on)

[sagikazarmark]

I would also consider adding immutability together with making it fluent.

[Nyholm]

I agree.
But adding immutability is a BC break.

[sagikazarmark]

I don't see why it's a BC break. configure returns an instance which is configured to do something. The fact that it's a new instance should not be a problem.

[Nyholm]

I consider it a BC break because this code will not work anymore:

$stargazers = $client->repo()->stargazers();
$stargazers->configure('star');
$stargazers = $stargazers->all('vuejs', 'vue');

It would be the same as:

$stargazers = $client->repo()->stargazers();
$stargazers = $stargazers->all('vuejs', 'vue');

[Nyholm]

Most of our APIs are stateful already. Lets make sure one can use a fluid interface. @Swader do you mind updating others APIs as well?

I've also added this to the 2.2 release. I would like to release that this Sunday.

[Swader]

@Nyholm I'll do my best but I'm traveling right now and through next week and will not be at a code-friendly outlet until some time next week, so might be unable to do it. Sorry in advance if I fail to do it soon.

[acrobat]

I will see if I can make some time for it this weekend and send a PR for it (of course if you don't have the time for it!)

[Swader]

@acrobat I'm 90% sure I'm not going to be able to make it - I'll be on my phone for 95% of the time, so no code. You're welcome to it!

[acrobat]

I've created #544 to make the missing configure methods chainable!