Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Intl][5.0] Add parameters type-hints#32525

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromtigitz:intl-params-type-hints
Aug 8, 2019
Merged

[Intl][5.0] Add parameters type-hints#32525

nicolas-grekas merged 1 commit intosymfony:masterfromtigitz:intl-params-type-hints
Aug 8, 2019

Conversation

@tigitz
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketscontinuation of#24722 and checks for#32179
LicenseMIT
Doc PRN/A

This PR replaces docblocks by type hints in the Intl component considering#32179. Some docblocks without valuable information got also removed.

[1.123,'1.1',1,1],
[1.123,'1.12',2,2],
[1.123,'1.123', -1,0],
[1.123,'1','abc',0],
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

'abc' is passed as thevalue of thesetAttribute method here:

https://github.com/symfony/symfony/pull/32525/files#diff-2a5ae15ec68c5d666ce08a09d75690efR564

It's type-hinted atint so it throws a TypeError, however method's logic seems to handle case where it could be a string:

* cast to Boolean and then to int again. This way, negative values are converted to 1 and string values to 0.

I decided to keep the type hint asint and remove the use case of astring value

nicolas-grekas reacted with thumbs up emoji
@tigitz
Copy link
ContributorAuthor

tigitz commentedJul 13, 2019•
edited
Loading

I decided at first to skip type-hints on methods throwingMethodNotImplementedException anyway.

But since#32525 (review) was asked I've decided to be consistent through the rest of the codebase:
61576fa

@xabbuhxabbuh added this to the5.0 milestoneJul 13, 2019
@tigitztigitz mentioned this pull requestJul 22, 2019
57 tasks
* @see https://en.wikipedia.org/wiki/ISO_4217#Active_codes
*/
publicfunctionformatCurrency($value,$currency)
publicfunctionformatCurrency(float$value,string$currency)
Copy link
Member

Choose a reason for hiding this comment

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

I‘m not sure if we should actually forcefloat here. Storing a decimal value as string is a valid pattern, especially when it comes to currency amounts.

Copy link
ContributorAuthor

@tigitztigitzAug 3, 2019•
edited
Loading

Choose a reason for hiding this comment

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

AFAIU, thisNumberFormatter implementation is a sort of polyfill forIntl::NumberFormatter where$value is typed as float and generates the following error if passed as a string with strict types:
Fatal error: Uncaught TypeError: NumberFormatter::formatCurrency() expects parameter 1 to be float, string given in

So question is, should we accept a wider range of type compared to the implementation it's trying to polyfill ?

Personally I would say no, I would expect this function to also throws a TypeError on string arg if the whole point of this class is to be as close as the "real" one.

I'll leave it type-hinted as a float for now, open to modification if team says otherwise.

derrabus reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

All right, thanks for checking.

@fabpot
Copy link
Member

@tigitz Can you have a look at the remaining comment here? Thank you.

@tigitz
Copy link
ContributorAuthor

@fabpot yep will do tonight 👌

xabbuh reacted with thumbs up emoji

@tigitz
Copy link
ContributorAuthor

tigitz commentedAug 3, 2019•
edited
Loading

Status: Needs Review

@derrabus When you have time ;)

@Tobion
Copy link
Contributor

Tobion commentedAug 4, 2019•
edited
Loading

It seems almost all classes in intl which are affected here are internal classes, e.g. for polyfill that are not meant to be extended. So we can add type hints in 4.4 to them because we don't need parameter variance from 7.2. What do you think@nicolas-grekas ?
And we could add return types as well without having to worry.

@nicolas-grekas
Copy link
Member

@Tobion I was about to answer yes, but./src/Symfony/Component/Intl/Resources/stubs/Collator.php contains a non-internal class, which extends the internalCollator. As such we cannot do more in 4.4.

Tobion reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestAug 8, 2019
…y of internal class (Tobion)This PR was merged into the 3.4 branch.Discussion----------[Intl] fix nullable phpdocs and useless method visibility of internal class| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets || License       | MIT| Doc PR        |Fix stuff found in#32525Commits-------63b71b5 [Intl] fix nullable phpdocs and useless method visibility of internal class
Copy link
Contributor

@TobionTobion left a comment

Choose a reason for hiding this comment

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

Rebased

@nicolas-grekas
Copy link
Member

Thank you@tigitz.

@nicolas-grekasnicolas-grekas merged commitce79f4b intosymfony:masterAug 8, 2019
nicolas-grekas added a commit that referenced this pull requestAug 8, 2019
…, Tobion)This PR was squashed before being merged into the 5.0-dev branch (closes#32525).Discussion----------[Intl][5.0] Add parameters type-hints| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | continuation of#24722 and checks for#32179| License       | MIT| Doc PR        | N/AThis PR replaces docblocks by type hints in the Intl component considering#32179. Some docblocks without valuable information got also removed.Commits-------ce79f4b [Intl][5.0] Add parameters type-hints
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@derrabusderrabusderrabus requested changes

+3 more reviewers

@pierreduppierreduppierredup left review comments

@seriquynhseriquynhseriquynh left review comments

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

9 participants

@tigitz@fabpot@Tobion@nicolas-grekas@pierredup@derrabus@seriquynh@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp