Skip to content

[String] Fix string to snake case conversion #49339

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

Closed

Conversation

Kolyunya
Copy link

@Kolyunya Kolyunya commented Feb 11, 2023

Q A
Branch? 6.3
Bug fix? ?
New feature? ?
Deprecations? no
Tickets
License MIT
Doc PR
Breaking change ‼️🚨‼️

Hi guys!

It seems to me that snake-case converter could be enhanced a bit.
Concretely, I believe that letters should be separated from numbers. What do you think?

Before: symfony5isGreatsymfony5is_great.
After: symfony5isGreatsymfony_5_is_great.

Notice that if you would decide to merge this, this is a breaking change.

@carsonbot carsonbot added this to the 6.3 milestone Feb 11, 2023
@Kolyunya Kolyunya force-pushed the fix-string-to-snake-case-conversion branch from 0d27885 to c307c58 Compare February 11, 2023 21:42
@Kolyunya Kolyunya changed the title Fix string to snake case conversion Enhance string to snake case conversion Feb 11, 2023
@OskarStark
Copy link
Contributor

I would rather go with:

symfony5_is_great

@Seb33300
Copy link
Contributor

@Kolyunya I think the case is wrong in your example.

It should be:
symfony5IsGreatsymfony5_is_great.

@Kolyunya
Copy link
Author

Kolyunya commented Feb 12, 2023

@Seb33300 my idea was that sequences of letters should be separated from sequences of numbers disregarding the capitalization of letters. So the example I provided is indeed intentional: symfony5isGreatsymfony_5_is_great.

@OskarStark please let me know if this is the final decision regarding the patch, I will update the tests and the production code accordingly.

Although, I believe that

  1. We should be separating parts of the string that are not semantically coherent. The separation should work word-wise. Symfony 5 are two words and they don't seem to be a single coherent lexical unit.
  2. In natural writing numbers never stick to the preceding letters. Example:

image

@OskarStark
Copy link
Contributor

OskarStark commented Feb 14, 2023

I agree, your example makes sense, but this sounds more like a bugfix to me, also I cannot see a BC break here.

@carsonbot carsonbot changed the title Enhance string to snake case conversion [String] Enhance string to snake case conversion Feb 14, 2023
@OskarStark OskarStark changed the title [String] Enhance string to snake case conversion [String] Fix string to snake case conversion Feb 14, 2023
@OskarStark
Copy link
Contributor

@nicolas-grekas WDYT?

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@fabpot
Copy link
Member

fabpot commented Aug 1, 2023

I don't think this is worth it as this is a BC break.

@javiereguiluz
Copy link
Member

Thanks for this proposal. I also think that we shouldn't do this change. In addition to the BC break, there are many legit cases (think of commercial product names, brands, etc.) where splitting by the numbers would be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants