Skip to content
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

Allow generators instead of deriving from strings in various Generator classes #74

Open
simon-mundy opened this issue Mar 24, 2021 · 5 comments

Comments

@simon-mundy
Copy link

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.

@Ocramius
Copy link
Member

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)

@simon-mundy
Copy link
Author

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?

@Ocramius
Copy link
Member

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.

@simon-mundy
Copy link
Author

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

@SVGAnimate
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants