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

Type hint on @return is confusing #126

Closed
driskell opened this issue Jul 22, 2021 · 1 comment
Closed

Type hint on @return is confusing #126

driskell opened this issue Jul 22, 2021 · 1 comment

Comments

@driskell
Copy link

As an example, below is the type hint for users.lookupByEmail

    /**
     * Find a user with an email address.
     *
     * @param array $queryParameters {
     *
     *     @var string $email An email address belonging to a user in the workspace
     *     @var string $token Authentication token. Requires scope: `users:read.email`
     * }
     *
     * @param string $fetch Fetch mode to use (can be OBJECT or RESPONSE)
     *
     * @return \JoliCode\Slack\Api\Model\UsersLookupByEmailGetResponse200|\JoliCode\Slack\Api\Model\UsersLookupByEmailGetResponsedefault|\Psr\Http\Message\ResponseInterface|null
     */
    public function usersLookupByEmail(array $queryParameters = [], string $fetch = self::FETCH_OBJECT)

However, it appears to be incorrect. I cannot see any instance where it would return ResponseInterface? Nor can I see any instance where it would return UsersLookupByEmailGetResponsedefault - indeed it seems that this is the result of calling getResponse() on a SlackErrorResponse exception. So indeed a @throws is missing perhaps.

Main issue is this causes significant issues with static analysis tooling because it believes the code is not properly handling the possible return types - for example it doesn't realise that UsersLookupByEmailGetResponsedefault will never be returned (which does not contain a getUser method) - nor that ResponseInterface will never be returned. Thus code becomes more complex or when somebody realises it's a false positive due to incorrect PHPDoc it causes lots of ignores.

Example in my case is with phpstan but I would guess would be the same for other tools like psalm too.

I guess this is perhaps caused by the Jane code generation but thought I'd just drop it here as perhaps there is a way to make it work. Removing the UsersLookupByEmailGetResponsedefault from the schema is one option and just having UsersLookupByEmailGetResponse200 the default one with all the fields - as you can still check the OK parameters and throw and use the same object - since the 200 has the user as nullable anyway. Though I guess that nullable technically is not the case for a successful response.

So yeh, interesting.

@driskell
Copy link
Author

Managed to work around this by adding type hints that do not include the default, ResponseInterface and null, since it seems none of those will ever return. Couldn't find a code path that resulted in a null return.

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

No branches or pull requests

1 participant