Fix method annotations in ClientInterface for lpush and similar

[kwidua]

I adjusted the second parameter of lpush and other methods where this also applies to be array|string ...$values instead of just array $values, in order to fix psalm warning.

Fixes #1146

[tillkruss]

@vladvildanov What the correct annotation for this?

[vladvildanov]

@tillkruss Are you talking about broken CI or...?

[tillkruss]

@tillkruss Are you talking about broken CI or...?

No, I mean array|string ...$fields. Does that not imply function($array, $array) would also work?

[vladvildanov]

@tillkruss In my opinion, we should only support string ...$fields and do not specify array at all, so user won't be confused and this fits original documentation https://redis.io/commands/hdel/. All the existing functionality won't break because under the hood we still support array, but we don't need to specify this and stick to original documentation 👌

[tillkruss]

@kwidua What do you think about just using string ...?

[kwidua]

What about everyone that has used array before, for them the psalm checks would fail then?

[tillkruss]

What about everyone that has used array before, for them the psalm checks would fail then?

I'm not sure if array|string ...$value works, doesn't that imply call_me([], []) must work as well? If so, we need a test case for that.

[vladvildanov]

@tillkruss Exactly, it means that it could be call_me([], []), that's why I'm against this approach. I believe that we need to support it the way it represented in Redis docs as string ...argument, yeah it will break IDE psalm check, but it will softly force people to change it in code with just prepend ... to existing array, we need to stop support incorrect interface.

P.S New commands support includes only ... approach if it's possible

[tillkruss]

I agree with @vladvildanov here, @kwidua. array|string ...$fields does not seem like a valid signature.

We can update the signatures to string ...$fields in the 3.x branch if you like.

If this is causing issues with your linters right now, feel free to update the test suite to resolve it in the 2.x branch.

[kwidua]

@tillkruss sorry for the delayed reply. If you prefer string ...$fields that's fine.
I don't quite understand what you mean by to update the test suite to resolve it in the 2.x branch. - can you maybe explain that? :)

[tillkruss]

@tillkruss sorry for the delayed reply. If you prefer string ...$fields that's fine.

No worries. string ... is great.

I don't quite understand what you mean by to update the test suite to resolve it in the 2.x branch. - can you maybe explain that? :)

Yes, I meant if any of the tests in the Predis suite need to be adjusted to please your local or CI linters, you can to adjust the code in tests/ as well.

[tillkruss]

@szepeviktor: What's the correct notation to support foo($array) as well as foo($string, $string, $string)?

[szepeviktor]

@tillkruss I am sorry. That is called bad design.
PHP type hints are not/will not be able to describe that after the first parameter ($array) there must not be a second nor a third parameter.

[tillkruss]

@kwidua @szepeviktor: Wouldn't the correct notation be this?

hdel(string $key, array|string $fields, string ...$extra_fields)

[szepeviktor]

No. Variable-length argument lists cannot be in union with something else.