laminas-code icon indicating copy to clipboard operation
laminas-code copied to clipboard

Allow generators instead of deriving from strings in various Generator classes

Open simon-mundy opened this issue 3 years ago • 5 comments

Referencing #30 and the issue with type-hinted parameters.

Currently the method for setting Type Hints in the ParameterGenerator enforces an object by deriving the TypeGenerator from a string.

     * @param  string $type
     * @return ParameterGenerator
     */
    public function setType($type)
    {
        $this->type = TypeGenerator::fromTypeString($type);

        return $this;
    }

It would be trivial and would not break existing code to change the method to allow a $type of either a string or a GeneratorInterface to be passed in, and therefore allow custom treatment of type hints (allowing aliases)

     * @param  string $type
     * @return ParameterGenerator
     */
    public function setType($type)
    {
        if (! ($type instanceof GeneratorInterface)) {
            $type = TypeGenerator::fromTypeString($type);
        }
        $this->type = $type;

        return $this;
    }

Would really appreciate this minor change being implemented - it creates as much work to re-code generated code to be less verbose when it could be easily avoided.

simon-mundy avatar Mar 24 '21 01:03 simon-mundy

Why would a GeneratorInterface be passed in in first place? I'd rather have a separate method, than expanding the parameter type (which breaks subclasses)

Ocramius avatar Mar 25 '21 19:03 Ocramius

Hi @Ocramius - for me it makes sense to pass it in as you're storing an instance of a GeneratorInterface anyway. It would not only be BC but closer to intent in terms of setters/getters.

I also think the benefits far outweigh the minor work needed to do this. I can happily write the additional unit test required if that would help?

simon-mundy avatar Mar 26 '21 01:03 simon-mundy

as you're storing an instance of a GeneratorInterface anyway.

That's a private/internal detail anyway.

I also think the benefits far outweigh the minor work needed to do this. I can happily write the additional unit test required if that would help?

Overall, I'm OK with passing in a TypeGenerator, but only on a dedicated method.

Reason for this is that we can then deprecate setType(), and keep setTypeGenerator() instead.

Ocramius avatar Mar 27 '21 13:03 Ocramius

Great I'll provide some code for review ASAP with the above in mind

simon-mundy avatar Mar 30 '21 00:03 simon-mundy

Hello,

Reason for this is that we can then deprecate setType(), and keep setTypeGenerator() instead.

Personally, I have my preference for the interface of @simon-mundy

     /**
     * @param  TypeGenerator|string|null $type
     * @return ParameterGenerator
     */
    public function setType(?TypeGenerator $type)

@see https://github.com/laminas/laminas-code/blob/4.12.x/src/Generator/PropertyGenerator.php#L344

But whatever, thank you

SVGAnimate avatar May 16 '23 04:05 SVGAnimate