Skip to content

Closes #177 - use the magic __call method for IDE completion #180

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

Merged
merged 5 commits into from
Aug 21, 2014
Merged

Closes #177 - use the magic __call method for IDE completion #180

merged 5 commits into from
Aug 21, 2014

Conversation

jbrooksuk
Copy link
Contributor

All of the new methods are camelCase. They have all of the same aliases too.

@GrahamCampbell
Copy link
Contributor

Why don't you just call the api function from the __call function?

@jbrooksuk
Copy link
Contributor Author

@GrahamCampbell that's not a bad idea. I could easily add the camel cased aliases too. WIll do that now.

@jbrooksuk
Copy link
Contributor Author

Done!

*
* @return ApiInterface
*
* @throws InvalidArgumentException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should throw a BadMethodCallException instead for __call when calling the wrong method (you should catch the InvalidArgumentException and throw another exception)

@stof
Copy link
Contributor

stof commented Aug 21, 2014

The usage of the magic api should be covered by a few tests too

@jbrooksuk
Copy link
Contributor Author

Where is the best place to test this? ClientTest.php?

@Nek-
Copy link
Contributor

Nek- commented Aug 21, 2014

The new way calling the old one. Not sure it's very cool. The __call method should be called by the old (and deprecated ?) api method...

[edit] and test in ClientTest.php ofc.

@jbrooksuk
Copy link
Contributor Author

@stof for the tests, would this make sense?

/**
 * @test
 * @dataProvider getApiClassesProvider
*/
public function shouldGetMagicApiInstance($apiName, $class)
{
    $client = new Client();

    $this->assertInstanceOf($class, $client->$apiName());
}

@stof
Copy link
Contributor

stof commented Aug 21, 2014

Where is the best place to test this? ClientTest.php?

yes

The new way calling the old one

well, if we decide that ->api($name) is deprecated, we should implement real methods rather than building it with __call. The question is whether you want to mark it as deprecated already

@jbrooksuk
Copy link
Contributor Author

What error message should be thrown by BadMethodCallException?

@jbrooksuk
Copy link
Contributor Author

  • Tests are added.
  • Catching of InvalidArgumentException which in turn throws BadMethodCallException.

@jbrooksuk
Copy link
Contributor Author

Make sense?

@jbrooksuk
Copy link
Contributor Author

@cursedcoder can we see this merged? :)

@cursedcoder
Copy link
Contributor

if @stof is ok with it

@stof
Copy link
Contributor

stof commented Aug 21, 2014

👍

@jbrooksuk
Copy link
Contributor Author

Awesome! :)

@stof
Copy link
Contributor

stof commented Aug 21, 2014

@cursedcoder Please bump the package version to 1.3.x when releasing this, to start followign semver for the version numbers (and I think it deserves being mentionned in the changelog of the release)

@GrahamCampbell
Copy link
Contributor

The branch alias is already for 1.3.

cursedcoder added a commit that referenced this pull request Aug 21, 2014
Closes #177 - use the magic __call method for IDE completion
@cursedcoder cursedcoder merged commit 414457e into KnpLabs:master Aug 21, 2014
@jbrooksuk jbrooksuk deleted the magic-api branch August 22, 2014 08:53
@LedzZm
Copy link

LedzZm commented Jun 6, 2022

I know I might be out of place, but I am going through the code, and am curious why this implementation was needed.

Why use magic methods here? Just for the aliasing, or is there any other reason I don't understand?
Thanks in advance for the info :)

I ended up here but it is not explained :D

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

Successfully merging this pull request may close these issues.

6 participants