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

Apply literal number separator rector #7299

Merged
merged 4 commits into from Jan 5, 2022

Conversation

weirdan
Copy link
Collaborator

@weirdan weirdan commented Jan 4, 2022

I opted for customary 3-digit groups, except version id, where 2-digit groups match the version id encoding.

I opted for customary 3-digit groups, except version id, where 2-digit
groups are used to match the version id encoding.
@weirdan weirdan added this to the Psalm 5 milestone Jan 4, 2022
@weirdan weirdan requested a review from orklah January 4, 2022 23:13
@weirdan weirdan added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jan 4, 2022
@AndrolGenhald
Copy link
Collaborator

It it possible to add this to the CS check so we ensure it stays consistent going forward?

@weirdan
Copy link
Collaborator Author

weirdan commented Jan 5, 2022

Not with the different grouping for version id. I find this difference quite useful, though.

@weirdan
Copy link
Collaborator Author

weirdan commented Jan 5, 2022

On the second thought, we can mandate at least one separator for values exceeding 1000 without specifying where it should be placed.

I would like to set minDigitsBeforeDecimalPoint to 4, but it causes
false positives with octal numbers (slevomat/coding-standard#1324).
@weirdan weirdan merged commit 0f84dd3 into vimeo:master Jan 5, 2022
@weirdan weirdan deleted the rector-literal-number-separator branch January 5, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants