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

Make UnitEnum name property non-empty #6964

Merged
merged 2 commits into from Nov 22, 2021
Merged

Conversation

ricardoboss
Copy link
Contributor

@ricardoboss ricardoboss commented Nov 22, 2021

No description provided.

@orklah orklah added the release:feature label Nov 22, 2021
@orklah
Copy link
Collaborator

orklah commented Nov 22, 2021

Thanks, that's great!

@orklah orklah merged commit 5b51569 into vimeo:master Nov 22, 2021
16 of 17 checks passed
@ricardoboss
Copy link
Contributor Author

ricardoboss commented Dec 5, 2021

Hi @orklah, it seems I made a mistake somewhere: https://psalm.dev/r/1bf1de16e0

Any idea why this PR didn't fix this?

@psalm-github-bot
Copy link

psalm-github-bot bot commented Dec 5, 2021

I found these snippets:

https://psalm.dev/r/1bf1de16e0
<?php

interface A {
    /**
     * @return non-empty-string
     */
    public function getName(): string;
}

enum B implements A {
    case Test;
    
    public function getName(): string
    {
        return $this->name;
    }
}
Psalm output (using commit 39402c2):

ERROR: ParseError - 10:6 - Syntax error, unexpected T_STRING on line 10

ERROR: ParseError - 17:1 - Syntax error, unexpected '}', expecting EOF on line 17

@orklah
Copy link
Collaborator

orklah commented Dec 5, 2021

Yeah, sorry, I should have thought about that, there's special code for handling ->name on enums here:

$statements_analyzer->node_data->setType($stmt, Type::getString());

I wonder if we could drop the whole block now that we have a stub... Do you mind trying that? If it doesn't work, then we need to replace Type::getString by a Type::getNonEmptyString()

@ricardoboss
Copy link
Contributor Author

ricardoboss commented Dec 5, 2021

I'll look into it!

@ricardoboss
Copy link
Contributor Author

ricardoboss commented Dec 5, 2021

So I tried flat-out removing the else branch, which led to an error in the tests saying "unable to infer return type". I then proceeded to change the line you mentioned to set the type to Type::getNonEmptyString() and that worked. Do you want to try to remove the special handling or is this fine as it is?

@orklah
Copy link
Collaborator

orklah commented Dec 6, 2021

that's fine, thanks for your help!

This was referenced Dec 7, 2021
This was referenced Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants