feat: Introduce `NumericLiteralSeparatorFixer` #6761

muuvmuuv · 2023-01-23T10:38:35Z

Closes: #6755

coveralls · 2023-01-23T14:31:10Z

coverage: 94.805% (+0.02%) from 94.789%
when pulling 29433f7 on muuvmuuv:feature-proposal-6755
into 4842c3e on PHP-CS-Fixer:master.

doc/rules/basic/numeric_literal_separator.rst

src/Fixer/Basic/NumericLiteralSeparatorFixer.php

tests/Fixer/Basic/NumericLiteralSeparatorFixerTest.php

kubawerlos

Can we have this fixer configured to add or remove the separator, according to the config?

Neither way is any standard that is good and the other is bad.

src/Fixer/Basic/NumericLiteralSeparatorFixer.php

tests/Fixer/Basic/NumericLiteralSeparatorFixerTest.php

muuvmuuv · 2023-01-23T19:03:46Z

Can we have this fixer configured to add or remove the separator, according to the config?

Neither way is any standard that is good and the other is bad.

Agree with it. Let's do:

true: add
false: remove
null: keep as is

I will try how easy it would be to do the other way around.

Is the position in the Fixer structure fine? Thought "basic" would match the most from what I saw.

keradus · 2023-01-23T23:50:19Z

not sure if agree, to be honest ;)

I would go YAGNI here. 123_345_678 is more readable than 123456789, I see way bigger value in fixer adding _ than in fixer removing _. To simplify the feature, I would suggest to have only adding(/fixing existing) _ and not removing them for first version of this Fixer rule, and consider to add option to remove existing _ only if more folks will actually need it.

src/Fixer/Basic/NumericLiteralSeparatorFixer.php

tests/Fixer/Basic/NumericLiteralSeparatorFixerTest.php

src/Fixer/Basic/NumericLiteralSeparatorFixer.php

kubawerlos · 2023-01-24T12:15:26Z

not sure if agree, to be honest ;)

I would go YAGNI here. 123_345_678 is more readable than 123456789, I see way bigger value in fixer adding _ than in fixer removing _. To simplify the feature, I would suggest to have only adding(/fixing existing) _ and not removing them for first version of this Fixer rule, and consider to add option to remove existing _ only if more folks will actually need it.

@keradus I would prefer to remove it, also I can imagine ignoring it, but I cannot imagine adding numeric literals automatically, how would you fix these automatically:

const ONE_EURO_IN_CENTS = 1_00;
const THOUSAND_EUROS_IN_CENTS = 1_000_00;
const HUNDRED_THOUSAND = 100_000;
const ONE_MILLION = 1_000_000;

(source)

muuvmuuv · 2023-01-24T15:32:28Z

@kubawerlos, in a first step, I remove the “maybe wrongly placed” underscores and then apply the formatting. So, that would become:

const ONE_EURO_IN_CENTS = 100;
const THOUSAND_EUROS_IN_CENTS = 100_000;

But I agree that some may prefer removing it or leaving as is. But as a first step, I want to make sure adding works as expected before removing.

keradus · 2023-01-29T13:33:19Z

@kubawerlos

A. why to remove _ from 12_345 to use 12345?

B. I see the value in having "add _ when not used" (convert 12345 into 12_345, but NOT touch 123_45 and option [false by default, can be added as later PR, if any] that would "rewrite _" (eg convert 123_45 into 12_345)

kubawerlos · 2023-01-29T15:21:52Z

@keradus my reasoning is if we cannot automate adding _ then we should not add it at all. The option to not touch the number if already has some separators is fine only if we automate checking the added are in the correct place.

muuvmuuv · 2023-01-29T17:46:17Z

Thats the reason I remove them first since you cannot be sure and one could misplaced them. I dont think someone wants them intentionally placed like that. I suggest to add it and wait for the community to ask for "remove", "keep as is" or possibilities to adjust the position etc.

keradus · 2023-01-29T22:18:58Z

I believe #6761 (comment) case shows that we must have "don't touch _ if already present" by default.

my reasoning is if we cannot automate adding _ then we should not add it at all.

Perfectly valid option, then just one not interested in it will not use the rule and that's it.

The option to not touch the number if already has some separators is fine only if we automate checking the added are in the correct place.

Can you elaborate on condition here? I believe this option is fine even if we don't automate checking - and because we cannot know what is the context of number, we will not be able to automate it.

keradus · 2023-01-30T20:58:23Z

I believe we can follow this path to make sure we are ready with state of fixer.
let's enable the fixer for this repo, and see if we will be happy with default config of it. If not, we need to groom it.

.vscode/settings.json

src/Fixer/Basic/NumericLiteralSeparatorFixer.php

doc/rules/basic/numeric_literal_separator.rst

github-actions · 2023-06-02T12:02:37Z

Since this pull request has not had any activity within the last 90 days, I have marked it as stale.

I will close it if no further activity occurs within the next 14 days.

Please keep your branch up-to-date by rebasing it when main branch is ahead of it, thanks in advance!

- use naming convention (testFix + provideFixCases) - do not check PHP version within provider, use `@requires PHP` - simplify test cases (get rid of `sprintf`) - fix PHPStan issues - apply current CS rules

Wirone

I'll let myself approve and merge this, as several things were already discussed, I've verified all open discussions, implemented missing pieces and fixed requested stuff. I believe it's good to go 🙂.

Wirone · 2024-01-10T18:51:42Z

Thank you very much @muuvmuuv 🍻!

keradus · 2024-01-10T20:17:16Z

doc/rules/basic/numeric_literal_separator.rst

+
+Allowed values: ``'no_separator'`` and ``'use_separator'``
+
+Default value: ``'no_separator'``


i'm not sure of the default.

It was suggested by me here and agreed by @kubawerlos so I implemented it like this. All the years I've been programming I have never worked with codebase that used literal separators, that's why I believe it's sane default.

well, it was not possible for long in PHP, no doubts there is not adoption.

I aim to use everywhere personally, basically a must have for me for TS or python.
if both of you think this set of defaults is good, let's keep the defaults as-is

Must personally agree with @keradus, but I am OK with it as long as I have the option to set it for myself.

Co-authored-by: Greg Korba <greg@codito.dev>

muuvmuuv changed the title ~~Insert numeric literal separator~~ feature: Insert numeric literal separator Jan 23, 2023