Skip to content

Loading…

@method tags on \Predis\Client #89

Closed
mvriel opened this Issue · 33 comments

7 participants

@mvriel

Dear @nrk,

Would you be opposed to having @method tags on the \Predis\Client class?
This could help IDEs apply auto-completion and for the redis expressions and to prevent IDEs from showing errors with regards to 'missing methods'.

@nrk
Owner
nrk commented

Hi @mvriel sorry for the late response.

I'm not really against having @method tags per se, but I wonder how to handle the fact that the argument list for the same redis command can vary (e.g. variadic commands can take string $key, array $values additionally to string $key, string $value1, string $value2, ...) and that developers can easily override these methods, in addition to the fact that server profiles for different versions of Redis do not expose the same commands (e.g. EVAL is available only when using the server profile for Redis 2.6, while GETRANGE was named SUBSTR before Redis 2.2).

In a sense having @method tags would help, but at the same time they could lead to confusion.... or is it just me?

@mvriel

No problem for the 'late' response, it is a feature request and nothing urgent ;)

I can understand the potential for confusion; and some things can be fixed and some could not. I personallly would like to have those @methods as these keep my IDE clean and help me discover which methods I need without having to rely on the Redis docs; yet at current @method does not support sub-tags with which you can indicate it is deprecated (though there is a feature request for that).

I'm biased pro-@method tags despite potential confusion but perhaps it would be nice to gather some opinions on this?

@nrk
Owner

In the meanwhile I will try with a bunch of commands and an IDE that uses hints from @method tags to get the feel of it.

@mvriel

Sounds cool!

@AlexeyKupershtokh

Hey @nrk? Any news on this issue?

@nrk
Owner

@AlexeyKupershtokh I approve of a separate project for this!

Let me know when you consider it ready and I'll put a link into the README of Predis.

@AlexeyKupershtokh

Sure.
It was mainly borrowed from the phpredis(-phpdoc) which has many incompatibilities with Predis. So it still requires thorough reviewing of documentation.
Though what's good about the predis-phpdoc is that it does not force developers to follow the docs if the docs are wrong.

@nrk
Owner

@AlexeyKupershtokh thinking about it, more than having a link in the README it would be reasonable to put your package in the require-dev list of composer.json in the future.

I left this issue open for months because I really wanted to propose a decent alternative to a list of @method tags but couldn't think of anything. I think this kind of compromise is good enough, @mvriel your opinion?

@mvriel

To be honest; I don't feel attracted to this solution. That is because the documentation in the code is now always wrong. When you @var a variable with a different class than it really is then that is bound to gather some confusion.

The example might be easy enough but let's pretend it is the return value of a method; or a random variable. Then I will not be able to find the original class.

What is wrong with a set of @method tags in the Class DocBlock itself?

@Danack

Hi,

I would also love to see this implemented. I use code inspection with the wonderful PHPStorm to help pick up the stupid errors I make in my code. Not having the methods tagged means that all uses of Predis generates inspection errors.

I wonder how to handle the fact that the argument list for the same redis command can vary (e.g. variadic commands can take string $key, array $values additionally to string $key, string $value1, string $value2, ...)

In general you can just set the extra parameters to accept null, and most editors will correctly show those variables as optional.

* @method int           decr(string $key, mixed $value1, mixed $value2 = null, mixed $value3 = null )

However, as variadics are coming in the PHP 5.6, I suggest just adding the "@ method"s now, and possibly tidying them up when they've been added to PHPDoc.

What is wrong with a set of @method tags in the Class DocBlock itself?

For the people who have made the poor life choice of making their code comments be semantically meaningful (aka use annotations) then there is a tiny performance hit, as OPCache needs to be told to load comments for the annotations to be available in code, so the more comments there are the bigger the file.

However the performance hit will be negligible compared to the programmer productivity bonus for having completely inspectable code.

btw although AlexeyKupershtokh's idea of having the documentation in a separate project is a nice idea, it doesn't work for me as I'm using real dependency injection to create my objects, along with the
DynamicReturnTypePlugin for PHPStorm, so the method documentation has to be on the correct object.

@nrk
Owner

@Danack

However, as variadics are coming in the PHP 5.6, I suggest just adding the "@ method"s now, and possibly tidying them up when they've been added to PHPDoc.

I was actually referring to how Predis by default handles Redis commands with variadic arguments. Just to make an example with DEL, both $client->del("key1", "key2") and $client->del(array("key1", "key2")) are supported ways to pass the keys for deletion.

We could decide to adopt the most obvious solution: documenting each command with @method tags following the same signature as defined by the Redis documentation. But how should we handle SET now that starting with Redis 2.8 its signature is SET key value [EX seconds] [PX milliseconds] [NX|XX]? In code it would look something like $client->set("key", "value", "EX", 10, "NX"). And wouldn't it be misleading when using Predis against versions of Redis < 2.8? We could include only the basic arguments required by each command, but then again developers might end up missing features of Redis until they check the docs on redis.io. There are some commands other than SET with a similar problem.

@mvriel

What is wrong with a set of @method tags in the Class DocBlock itself?

There's nothing wrong, but confusion related to versioning and optional arguments still worries me. Using a separate class with the @var trick only for documenting these methods could solve the versioning issue by providing multiple classes, each one targeting a specific Redis version. That doesn't mean it's a good solution anyway.

I'd like to settle this issue before tagging a stable release of the next major version (could happen in a month or so), any further feedback would be greatly appreciated.

@Danack

As I said, my desire to have this is so that I can use code inspection tools against my code, which would require the documentation should cover all the use cases possible, even if these are not possible for all versions of Redis.

So in my view :

both $client->del("key1", "key2") and $client->del(array("key1", "key2"))

should be documented as:

bool          del(mixed $key1OrArray, mixed $key2 = null, mixed $key3 = null, mixed $key4 = null)

to allow all the use cases to be allowed. Exactly how many keys to allow is not an easy question to answer - but I meant that when PHP has variadics, you would be able to tidy up the documentation to:

bool          del(mixed $key1OrArray, mixed $key2 = null, ...)

Or whatever the syntax will be.

For the other one:

In code it would look something like $client->set("key", "value", "EX", 10, "NX") - We could include only the basic arguments required by each command, but then again developers might end up missing features of Redis

Again, if people want to use that feature to avoid having a false positive problem for the code inspection it would have to be written as permissively as possible.

bool          set(string $key, string $value, int $expireTimeSeconds = null, int $expireTimeMilliseconds, $flags = null);

And wouldn't it be misleading when using Predis against versions of Redis < 2.8?

My personal view is that:

i) The docblock comments are there as a tool to help you write programs. They aren't there to help people design programs, so if people are basing which features they expect to use on the Predis docblocks rather than the official Redis documentation, then that's their problem. However the docblock comments with code inspection has say valid code is valid, otherwise it's really defeating the purpose of having the docblock comments.

ii) Supporting the latest version completely is more important for the documentation than continuing to support older versions, as people just starting to use Predis/Redis are likely to use the latest version. It is only people with existing infrastructure that use older versions of Redis and are unable to upgrade that could possibly be confused - but by definition they have stable code bases which they are not wanting to upgrade.

cheers
Dan

@nrk
Owner

@Danack, @mvriel

I've finally tried adding @method tags in a commit on a separate branch based on master and all in all I think I reached a decent compromise. In the end I decided to follow only the Predis-specific variant of the commands API because:

  • The @method tag is geared towards PHP after all, trying to stick with the signature as defined by Redis would have ended up in some messy documentation for certain commands (e.g. ZINTERSTORE or SORT) .
  • I used phpstorm to see the actual result and this IDE seems to not complain when straying from the signature specified by @method. Just to make an example, $client->sadd("foo", "member1", "member2") is OK even if SADD is defined as sadd($key, array $members). I don't know how other IDEs behave though.

Do you have any suggestion for improvements or changes?

At this point I guess that @method tags should also be added to Predis\Pipeline\Pipeline and Predis\Transaction\MultiExec since they rely on __call() too...

@Danack

Hi @nrk,

That looks pretty good to me. The only thing I'd note is that two of the tests use an int instead of a string, and so throw an inspection error:

* @method int    zunionstore($destination, array $keys, array $options = null)
$redis->zunionstore('zset:destination', 1, 'foo');


* @method int    zinterstore($destination, array $keys, array $options = null)
$redis->zinterstore('zset:destination', 1, 'foo');

cheers
Dan

@nrk
Owner

@Danack you were using PHPStore, right? Because I don't see any notice with PHPStorm 7.1 if I open the tests of both zunionstore and zinterstore.

By the way I was trying to document the pipeline and transaction classes which both have a fluent interface returning themselves to chain calls and I read on the phpdoc docs that in such cases $this should be used, but PHPStorm doesn't seem to understand it e.g. @method $this del(array $keys) doesn't work. Is it just me? Am I doing something wrong?

@Danack

you were using PHPStorm, right?

Yes, though version 7.0 and only when I run the "Inspect code" tool that does the full code inspection. It doesn't indicate any error when just editing.

I read on the phpdoc docs that in such cases $this should be used, but PHPStorm doesn't seem
to understand it e.g. @method $this del(array $keys)

I see the same thing. It looks like in both PHPStorm and Eclipse you have to be explicit with the return type, specifying the actual class, e.g.

* @method SomeClass foo(array $keys)

which could be annoying. The 'correct' syntax would probably be a return type of 'static' to return the 'late static binding name' but no tools seem to support that at all.

@nrk
Owner

Yes, though version 7.0 and only when I run the "Inspect code" tool that does the full code inspection. It doesn't indicate any error when just editing.

Oh I see, indeed when running inspect code I see a lot of warnings (some meaningful which are worth getting fixed, others not).

I see the same thing. It looks like in both PHPStorm and Eclipse you have to be explicit with the return type, specifying the actual class, e.g.

Even when explicitly using the class name, which would translate to @method Pipeline del(array $keys) in my example above, I get completion only for the first method call but the rest of the chain is not recognized so when doing $pipeline->del($keys)->del($keys) only the first del() is OK for the IDE so it's kind of broken anyway.

which could be annoying. The 'correct' syntax would probably be a return type of 'static' to return the 'late static binding name' but no tools seem to support that at all.

Yes, I also tried using static but with no luck.

@nrk
Owner

Actually $this and static seems to be correctly supported by PHPStorm when they are used with @return in classic phpdoc headers put above methods, so I guess it's just that they are not recognized when using @method (a bug maybe?).

@Danack

The below seems to work for me:

/**
 * This is some class.
 * 
 * @method SomeClass del(array $keys)
 */
class SomeClass {
    function foo(){}
}

$testClass = new SomeClass();

$keys = array();

$endVar = $testClass->del($keys)->del($keys);

$endVar->foo();

With $endVar correctly being recognised as of type 'SomeClass'.

Actually $this and static seems to be correctly supported by PHPStorm when they are used with @return in classic phpdoc headers put above methods, so I guess it's just that they are not recognized when using @method (a bug maybe?).

Hmm, definitely possibly a bug, with it just not scoping $this/static to be of the class that follows, when they're used outside the class.

indeed when running inspect code I see a lot of warnings

Yes, it's a useful tool :)

@nubs

I would agree that the docblock would be useful. Not just for IDEs, but for helping out tools like PHP Analyzer. Here's an example of what PHP Analyzer thinks when it doesn't find the method it was looking for: https://scrutinizer-ci.com/g/dominionenterprises/memoize-php/inspections/05e2bd68-6c7e-4ff3-ae4f-7b0915264cef/issues/files/src/Predis.php?status=new&orderField=path&order=asc

@Danack

Hi,

Any chance of tagging this branch as a release version?

cheers
Dan

This was referenced
@nrk nrk added a commit that referenced this issue
@nrk Add phpdocs for Redis commands in Predis\ClientInterface.
This commit addresses a long-standing feature request posted in #89.
7d78600
@nrk
Owner

OK guys I need feedback about commit 7d78600 using an IDE (it's in the v1.0/phpdocs branch) and some help to solve this for pipelines and transactions, if using $this in @method tags is still an issue in current IDEs.

I don't use an IDE so I have to download and configure again a trial of PhpStorm or something like that, but can't do it until the weekend. I'd love to ship Predis v1.0.0 on August 1st with @method added to phpdocs, and eventually backport this branch to v0.8 for the next patch release. \cc @Danack @nubs @mvriel

@GrahamCampbell

Cool. This use useful for projects using php anyalyser on scrutinizer-ci too.

@Danack

Hi @nrk,

if using $this in @method tags is still an issue in current IDEs.

Using $this appears to work fine in PHPStorm 8 EAP. 'self' works but has a weird syntax error in the comment block, and static still doesn't work, which has been reported So I think if you just put the appropriate tags in the Pipeline class using $this it will work fine. If you put them there, and ping me again, I'll test it again.

Just to note, there's also a bug in the inspection code for PHPStorm but that shouldn't affect you.

@nrk
Owner

@Danack OK I'll try using $this then.

I think I'll add @method tags to what's now known as Predis\ExecutableContextInterface which is implemented by both Predis\Pipeline\Pipeline and Predis\Transaction\MultiExec. This interface will be renamed to Predis\ClientContextInterface and I will add the __call() magic method to it. I'm not sure if this will trigger the bug you mentioned, but we'll see.

I'll ping you back as soon as I push this change into the above mentioned branch!

@Danack

Just to note one thing, the type-hinting in PHPStorm is done in a slightly interesting way http://youtrack.jetbrains.com/issue/WI-18595

This shouldn't affect people using the library, but if you ever do code inspection with PHPStorm on Predis, PHPStorm will complain about missing methods on the class, unless you (slightly ridiculously) duplicate the docblock containing the '@ methods' from the interface.

I suggest just ignoring that, everything works just fine with the methods in the docblock on the interface. Although having comment annotations isn't a massive burden, duplicating them would be a tiny more load for no apparent benefit for anyone using the library.

@nrk
Owner

@Danack I just pushed an update on the same branch.

I chose to add @method tags to Predis\ClientContextInterface (which is common to both Predis\Pipeline\Pipeline and Predis\Transaction\MultiExec) because now it defines the __call() method, so I'd expect to see them there.

I wonder how php-anyalyser behave...

@nrk
Owner

@Danack I just tried using PHPStorm 8 EAP with this branch and things look good to me.

Also, I tried running a code inspection on Predis and PHPStorm ends the inspection with No suspicious code found which I think it's great news! I'd still like to receive confirmation from you, just to make sure that I got everything right :-)

@Danack

Hi @nrk ,

The Pipeline and MultiExec classes are working fine for me, with full chained type-hinting, which is very nice.

However I think you may have misconfigured the inspection tool for PHPStorm. Although it gives some definitely spurious warnings, e.g. unused variable when the code is doing:

list($iDontCareAboutThisVar, $iDoWantThisVar) = foo();

It is is reporting what appear to be actual problems as well, a sample of which are:

Undefined class ClusterInterface

Undefined class - looks like missing use

As well as stuff which are either not issues at all or just things that need tidying:

Unused parameter $parameters

Unused parameter $resource

Unused private fields $OK and $QUEUED - technically not a problem just missed tidy up.

@nrk
Owner

Hah, it seemed too good to be true. I've just tried again from scratch and now I can see all kind of warnings being emitted.

Anyway since the initial purpose of that branch has been reached, I think I can merge everything into master and fix the reports there. This is probably going to make you guys happy :-)

Regarding the reports you listed:

Undefined class ClusterInterface

Fixed after merge commit 558e109 (the phpdoc branch is not up to date with master).

Undefined class - looks like missing use

Whoops, fixing it now.

Unused parameter $parameters

It's indeed useless, will fix.

Unused parameter $resource

This is due to the method signature required by CURLOPT_WRITEFUNCTION.

Unused private fields $OK and $QUEUED

Considered the trick I used to avoid duplicating code I'm not surprised it thinks those two private fields are unused :-)

Thanks a lot @Danack!!

@nrk
Owner

Just merged branch v1.0/phpdocs into master! I will look into backporting the same stuff to v0.8 for the next patch release, if possible.

Thanks everyone for the patience :-)

@nrk nrk closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.