Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Improve handling of digits in CamelCaseToSeparator filter #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edigu
Copy link

@edigu edigu commented Oct 5, 2016

This PR aims to add the behaviour asked in #33

Since the reason behind using the CamelCaseSeparator is tokenizing strings when case is changed, usual expectation is beginning a new token when hitting a number or digit.

I will try to summarize both current and changed behaviour on numbers with some examples:

Original Text Current After this PR
Foo2016Bar  Foo2016-Bar Foo-2016-Bar
March16Events March16-Events  March-16-Events
DigitSuffix42 Digit-Suffix42 Digit-Suffix-42
42metersLong 42metersLong  42-meters-Long

Notice the last example: Beginning of the word with lowercase letter after 42 is also mimics a case-change. I'm still not sure is this a acceptable/desired behaviour or not.

The issue #33 is about the CamelCaseToDashFilter but since it derived from CamelCaseToSeparator like CamelCaseToUnderscore, this change will affect both childs of CamelCaseToSeparator. For this reason, I also added more tests to CamelCaseToDashTest.

After digging dozen of approaches including other platforms such as Java and C# I came up with this final patterns with positive lookbehind and lookaheads. (Achieving same effect with a single regex seems like impossible, at least I give up)

I'm also aware of changing an existing test is not a good practice. Since the expected behaviour is changed for digits, I forced into touching the CamelCaseToUnderscoreTest:

  • Pa2_Title => Pa_2_Title
  • Pa2a_Title => Pa_2a_Title

Altering the CamelCaseToUnderscoreTest seems like mandatory if the desired behaviour will be changed.

All tests are still green.

@weierophinney
Copy link
Member

I'm pushing this to 3.0 (which will happen this year, as we will be releasing new major versions of all components that set PHP 7.1 as the minimum supported version). While I agree it is the correct behavior, I worry that if users are expecting the current behavior currently, this would break their applications. Pushing it to a new major version allows them to upgrade at their own pace.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-filter; a new issue has been opened at laminas/laminas-filter#9.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-filter. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-filter to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-filter.
  • In your clone of laminas/laminas-filter, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

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

Successfully merging this pull request may close these issues.

None yet

3 participants