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

Fix Rate RequestOption and type-hinting #349

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chrom
Copy link

@chrom chrom commented May 17, 2022

No description provided.

src/Rate.php Outdated
*/
public function shopRates($rateRequest)
public function shopRates($rateRequest, string $requestOption = 'Rate'): RateResponse
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a class with all requestoption strings? (old school enum)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are talking about a list of possible parameter values, then it is specified \Ups\Rate::setRequestOption in comment
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but you can create kind of this class. It forces type-hinting instead of relying on pure string while we don't have a good PHPStan coverage.

final class RateRequestOption
{
	public const RATE = 'Rate';
	public const SHOP = 'Shop';
	// ...
}

and use it in function definition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/Ups.php Show resolved Hide resolved
@chrom
Copy link
Author

chrom commented May 18, 2022

IMHO we need add how to run phpstan for some file in example Rate.php to doc:
./vendor/bin/phpstan analyse ./src/Rate.php -c ./phpstan.neon --level 8 -a ./vendor/autoload.php

@ptondereau
Copy link
Collaborator

PHPStan integration is planned but we need to have a full pass on current docblock because some classes are not up-to-date or typehint is wrong.

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

Successfully merging this pull request may close these issues.

2 participants