Deprecated precision option in favor of scale by wouterj · Pull Request #13717 · symfony/symfony

wouterj · 2015-02-17T13:24:34Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#7383
License	MIT
Doc PR	symfony/symfony-docs#5005

Scale is the number of digits to the right of the decimal point in a number, precision isn't. See the referenced ticket for more context.

stof · 2015-02-17T13:38:21Z

renaming a protected property is a BC break (we should probably switch it to private in 3.0 btw)

wouterj · 2015-02-17T19:33:43Z

Thanks @stof. I've applied some fixes to this PR.

wouterj · 2015-02-17T20:01:13Z

failures look unrelated

fabpot · 2015-02-18T10:17:32Z

👍

nicolas-grekas · 2015-02-18T10:21:15Z

// @deprecated rename to scale in Symfony 3.0

If we want forward compat, we should add $scale right now (and keep $precision in sync for BC)

How can we make 2 properties in sync with eachother?

alias them by reference ?

But we could also make all props private in 3.0 as @stof suggested. Would be even better

In that case, you can't make your code future compatible as you can no longer extend this class for that purpose?

right, then only the comment needs an update, e.g // @deprecated: will be replaced by a $scale private property in 3.0

Tobion · 2015-02-18T10:42:49Z

Please configure the allowed types (null|int) for scale with setAllowedTypes.

wouterj · 2015-02-18T14:02:47Z

I've fixed all remaining comments and created a doc PR. Ready to merge imo ✅

nicolas-grekas · 2015-02-18T15:29:04Z

One last comment sorry :) /** here?
But more importantly (for @stof or @Tobion): shouldn't we also deprecate the $grouping and $roundingMode props and make the private in 3.0?

Not sure about the deprecation at all. The class is meant to be extended (and also is extended in symfony itself). This why they are protected. It's just currently the case, that the properties are not needed to be read in subclasses. But making them private makes this impossible.

But I agree deprecating all makes more sense than just one.

fabpot · 2015-02-25T11:34:52Z

The idea lLooks good to me. @wouterj Can you address the comments?

wouterj · 2015-02-27T16:52:46Z

@fabpot I'm a bit unsure what to do:

Just change /* to /**
(1) + deprecate all other properties
(1) + changing message to only mention rename (and not change to private) + adding renamed property for future compatibility

wouterj · 2015-03-20T18:41:36Z

ping

wouterj · 2015-04-03T14:32:08Z

For now, I've done (1). If I needed to do anything else, please say so :)

Tobion · 2015-04-03T15:14:47Z

👍

aitboudad · 2015-04-03T15:22:16Z

👍

fabpot · 2015-04-03T17:01:56Z

Thank you @wouterj.

This PR was merged into the 2.7 branch. Discussion ---------- Deprecated precision option in favor of scale | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #7383 | License | MIT | Doc PR | symfony/symfony-docs#5005 Scale is the number of digits to the right of the decimal point in a number, precision isn't. See the referenced ticket for more context. Commits ------- 2a2f7e2 Deprecated precision option in favor of scale

This PR was merged into the 2.7 branch. Discussion ---------- Renamed precision option to scale | Q | A | --- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#13717) | Applies to | 2.7+ | Fixed tickets | - Commits ------- 59ac3d5 Renamed precision option to scale

… (WouterJ) This PR was merged into the 2.8 branch. Discussion ---------- [2.8] [Form] Rename CollectionType options for entries Description --- Replaces #13820 for the 2.8 branch. Original description: > `type` and `options` are extremely generic. Prefixing them with `entry_` makes it clear what they are configuring. > About the property deprecation it is the same story as #13717 and I don't know which direction you want me to go. I've tried to apply the comments in the previous PR, but got a bit lost in the normalizers/default closure stuff. I hope I did everything correctly, but please review :) PR Info Table --- | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #7831 | License | MIT | Doc PR | symfony/symfony-docs#5051 Commits ------- 942a237 Rename CollectionType options for entries

… (WouterJ) This PR was merged into the 2.8 branch. Discussion ---------- [2.8] [Form] Rename CollectionType options for entries Description --- Replaces #13820 for the 2.8 branch. Original description: > `type` and `options` are extremely generic. Prefixing them with `entry_` makes it clear what they are configuring. > About the property deprecation it is the same story as symfony/symfony#13717 and I don't know which direction you want me to go. I've tried to apply the comments in the previous PR, but got a bit lost in the normalizers/default closure stuff. I hope I did everything correctly, but please review :) PR Info Table --- | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #7831 | License | MIT | Doc PR | symfony/symfony-docs#5051 Commits ------- 942a237 Rename CollectionType options for entries

stof reviewed Feb 17, 2015
View reviewed changes

wouterj force-pushed the issue_7383 branch from 3c41a9c to bda7107 Compare February 17, 2015 19:33

nicolas-grekas reviewed Feb 18, 2015
View reviewed changes

wouterj force-pushed the issue_7383 branch 3 times, most recently from 93b74d6 to f0ee681 Compare February 18, 2015 14:01

wouterj mentioned this pull request Feb 18, 2015

Renamed precision option to scale symfony/symfony-docs#5005

Merged

nicolas-grekas reviewed Feb 18, 2015
View reviewed changes

fabpot added the Form label Feb 25, 2015

wouterj mentioned this pull request Mar 1, 2015

[Form] Rename CollectionType options type and options #13820

Closed

Deprecated precision option in favor of scale

2a2f7e2

wouterj force-pushed the issue_7383 branch from f0ee681 to 2a2f7e2 Compare April 3, 2015 14:31

fabpot merged commit 2a2f7e2 into symfony:2.7 Apr 3, 2015

wouterj deleted the issue_7383 branch April 3, 2015 17:28

wouterj mentioned this pull request Jun 18, 2015

[2.8] [Form] Rename CollectionType options for entries #15025

Merged

Uh oh!

Conversation

wouterj commented Feb 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commented Feb 17, 2015

Uh oh!

wouterj commented Feb 17, 2015

Uh oh!

fabpot commented Feb 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Feb 18, 2015

Uh oh!

wouterj commented Feb 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Feb 25, 2015

Uh oh!

wouterj commented Feb 27, 2015

Uh oh!

wouterj commented Mar 20, 2015

Uh oh!

wouterj commented Apr 3, 2015

Uh oh!

Tobion commented Apr 3, 2015

Uh oh!

aitboudad commented Apr 3, 2015

Uh oh!

fabpot commented Apr 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants