Add the possibility to set the minimum number of decimals in the numb… #113
Conversation
MatthiasKuehneEllerhold
force-pushed
the
numberformatter_min_decimals_2_24
branch
2 times, most recently
from
b457165 to
5294806
Compare
|
Added Unit-Tests and fixed CI (hopefully). DCO still says "signoff missing" although all commits are signed by my GPG key. What did I do wrong? |
There was a problem hiding this comment.
Thanks @MatthiasKuehneEllerhold - Generally speaking, whilst a desirable improvement, adding the native property/return/parameter types to existing props and methods all break BC. Because we're targeting a minor, they will have to be reverted and doc-blocks reinstated. Sorry!
src/View/Helper/NumberFormat.php
Outdated
| * The maximum number of decimals to use. | ||
| */ | ||
| protected ?int $maxDecimals = null; |
There was a problem hiding this comment.
This newly added property can be private - ideally, all of the properties in this class would be private, but it is not possible to make them so without breaking BC - this is because the class has not been marked as final and is therefore open to inheritance.
There was a problem hiding this comment.
Ive added them as "protected" to keep consistent with the other attributes.
What I like about the zend framework then and laminas now is that I can overwrite and change classes and change behavior in subtle ways. Ideally I just overwrite a small piece of a class instead of copy and paste everything in a new class.
Thats why I'm personally in favour of making everything protected (and therefore accessable to inherited classes) instead of privating everything and forcing myself to copy-and-pasting whole classes.
src/View/Helper/NumberFormat.php
Outdated
| /** | ||
| * Get the maximum number of decimals. | ||
| */ | ||
| public function getMaxDecimals(): ?int | ||
| { | ||
| return $this->maxDecimals; | ||
| } | ||
|
|
||
| /** | ||
| * Get the minimum number of decimals. | ||
| */ | ||
| public function getMinDecimals(): ?int | ||
| { | ||
| return $this->minDecimals; | ||
| } |
There was a problem hiding this comment.
Are these getters necessary? It's better not to add new public methods when the effect of the new minDecimals property can be tested without verifying the value of the internal property.
There was a problem hiding this comment.
Everything else had a getter, so I added them.
Are they necessary though? I have a few situations where I get all internal state from an object, change it and then set the old state back. e. g. while rendering a view script I get the old locale from a lot of entities, set the desired locale, render my PHTML and then re-set the old locale.
Im not sure this situation would ever arise here though. But I erred on the side of caution and added them.
|
I could add another PR with all the type hint changes, this time targeting a more recent branch - if you like. All it would take is a git revert of ae1da38 (and possibly some more work in the other files ;-) ). |
…er formatter Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
Signed-off-by: Matthias Kühne <[email protected]>
MatthiasKuehneEllerhold
force-pushed
the
numberformatter_min_decimals_2_24
branch
from
de362e2 to
06a20fd
Compare
Description
We have some user provided numbers that may have zero to
endless decimals. We want to display them in the user locale exactly as
provided.
If the user inputs "1", we want to display "1". If the user inputs "0,5432" (german notation!) we want to display "0,5432".
Sadly with the current (2.24.1) View-Helper we have to know beforehand how many decimals the user has used.
The underlying NumberFormatter-Class can do this: by
setting min-decimals to zero and max-decimals to PHP_INT_MAX (or some
large arbitrary number). The view-helper does not allow to set these 2
options separatly (yet). Thats what I added.
According to the github-template I should target the
"develop" branch. Sadly no branch named "develop" can be found in this
repository, so I've targeted the branch of the currently active minor
version. Please advice if this was the right approach.
Some commits are optional: adding type hint to variables and classes and may be discard if undesired.