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

Bug in Underscore Namingstrategy #101

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@boesing
Copy link
Contributor

boesing commented Mar 14, 2019

Hey guys,

in v2, the failing unit test worked as expected.
Since v3, the CamelCaseToUnderscoreFilter adds various underscores around the numeric values within a string.

Any suggestion how this can be fixed without breaking the other tests?

And why was that behavior changed that much?
CamelCaseToSeparator of zend-filter works a lot different than the new logic.

I dont even get the new logic:

-'bfd7b82e9cfceaa82704d1c1_foo'
+'bfd_7_b82e_9_cfceaa_82704_d1c_1_foo'

So why the string is being separated with _ after bfd and not after bfd7b again? x_O

@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Mar 14, 2019

I dont even get the new logic:

-'bfd7b82e9cfceaa82704d1c1_foo'
+'bfd_7_b82e_9_cfceaa_82704_d1c_1_foo'

So why the string is being separated with _ after bfd and not after bfd7b again? x_O

It looks like it's separating at numeric boundaries... but, honestly, if that were 100% the case, I'd expect bfd_7_b_82_e_9_cfceaa_82704_d_1_c_1_foo.

What happened for v3 was that we brought the filters into the package, to remove the dependency on zend-filter. It looks like there are differences between zend-filter and what we do here, though, so I'll review the two side-by-side to figure out how and why, and fix it.

Thanks for the failing test!

@boesing

This comment has been minimized.

Copy link
Contributor Author

boesing commented Mar 14, 2019

It looks like it's separating at numeric boundaries... but, honestly, if that were 100% the case, I'd expect bfd_7_b_82_e_9_cfceaa_82704_d_1_c_1_foo.

Thats exactly what I thought, yup.

@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Mar 14, 2019

@boesing Can you enable maintainer push rights on your fork? I have a patch I've built against your branch that contains a fix, and I'd like to push back to your branch.

Thanks!

@boesing

This comment has been minimized.

Copy link
Contributor Author

boesing commented Mar 15, 2019

@weierophinney edits from maintainers are enabled in this PR or do I have to do anything else?

@webimpress

This comment has been minimized.

Copy link
Contributor

webimpress commented Mar 15, 2019

@boesing On the side in your PR just check the checkbox:

image

@boesing

This comment has been minimized.

Copy link
Contributor Author

boesing commented Mar 15, 2019

@boesing On the side in your PR just check the checkbox:

image

Thats already activated.
image

@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Mar 15, 2019

Really strange - it won't let me push, citing access rights.

I'll push to my own branch and open a new PR, closing this one.

@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Mar 15, 2019

#103 builds on your tests; thanks, @boesing !

@boesing boesing deleted the boesing:bugfix/underscore-naming-strategy branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.