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

[Serializer] CamelCaseToSnakeCaseNameConverter upper camel case not working #21399

Closed
markusu49 opened this Issue Jan 25, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@markusu49
Contributor

markusu49 commented Jan 25, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version all supported

The CamelCaseToSnakeCaseNameConverter provides a constructor argument to specify whether to use lower camel case or not. Denormalizing upper camel case works fine, but normalizing ThisIsATest returns _this_is_a_test (should be this_is_a_test).

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 25, 2017

Contributor

@markusu49 this is happening because ThisIsATest is not camelCase but PascalCase, see https://3v4l.org/qTioM.

Contributor

theofidry commented Jan 25, 2017

@markusu49 this is happening because ThisIsATest is not camelCase but PascalCase, see https://3v4l.org/qTioM.

@markusu49

This comment has been minimized.

Show comment
Hide comment
@markusu49

markusu49 Jan 25, 2017

Contributor

Isn't this naming Microsoft-specific (see Wikipedia)? What is the lowerCamelCase attribute for then?

Contributor

markusu49 commented Jan 25, 2017

Isn't this naming Microsoft-specific (see Wikipedia)? What is the lowerCamelCase attribute for then?

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 25, 2017

Contributor

Some people and organizations, notably Microsoft,[4] use the term camel case only for lower camel case.

It's not limited to Microsoft but more generally in programming languages.

Contributor

theofidry commented Jan 25, 2017

Some people and organizations, notably Microsoft,[4] use the term camel case only for lower camel case.

It's not limited to Microsoft but more generally in programming languages.

@markusu49

This comment has been minimized.

Show comment
Hide comment
@markusu49

markusu49 Jan 25, 2017

Contributor

PascalCase is just another name for upper camel case - people often call things differently. However, the denormalize method supports it:

return $this->lowerCamelCase ? lcfirst($camelCasedName) : $camelCasedName;

Why shouldn't the normalize method, too?

Contributor

markusu49 commented Jan 25, 2017

PascalCase is just another name for upper camel case - people often call things differently. However, the denormalize method supports it:

return $this->lowerCamelCase ? lcfirst($camelCasedName) : $camelCasedName;

Why shouldn't the normalize method, too?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jan 25, 2017

Member

@theofidry as your last quote points it out, there is indeed two kind of camel case: lower and upper, PascalCase being a synonym of upper camelCase (also known as StudlyCaps). I was not aware of that but according to https://fr.wikipedia.org/wiki/CamelCase, it seems true.

The $lowerCamelCase constructor argument let me understand the same as @markusu49 i.e. you can chose between both syntaxes using this normalizer so the result it expects looks legit to me.

Member

chalasr commented Jan 25, 2017

@theofidry as your last quote points it out, there is indeed two kind of camel case: lower and upper, PascalCase being a synonym of upper camelCase (also known as StudlyCaps). I was not aware of that but according to https://fr.wikipedia.org/wiki/CamelCase, it seems true.

The $lowerCamelCase constructor argument let me understand the same as @markusu49 i.e. you can chose between both syntaxes using this normalizer so the result it expects looks legit to me.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 25, 2017

Contributor

@chalasr IMO it's up to discussion, and tbh I don't feel strongly about it, but if the denormalize method supports it, the normalize one should as well indeed.

Contributor

theofidry commented Jan 25, 2017

@chalasr IMO it's up to discussion, and tbh I don't feel strongly about it, but if the denormalize method supports it, the normalize one should as well indeed.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jan 25, 2017

Member

Also I would intuitively prefer a CamelCaseToSnakeCaseNameConverter and a PascalCaseToSnakeCaseNameConverter than only the former handling both

Member

chalasr commented Jan 25, 2017

Also I would intuitively prefer a CamelCaseToSnakeCaseNameConverter and a PascalCaseToSnakeCaseNameConverter than only the former handling both

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Jan 25, 2017

Contributor

Isn't "lower camel case" simply "camel case", whereas "upper camel case" is "pascal case"? The class/arguments are unclear in their current form. Two separate converter classes would be more appropriate.

Edit: We posted at the same time, but indeed, @chalasr, that would be much more intuitive.

Contributor

robfrawley commented Jan 25, 2017

Isn't "lower camel case" simply "camel case", whereas "upper camel case" is "pascal case"? The class/arguments are unclear in their current form. Two separate converter classes would be more appropriate.

Edit: We posted at the same time, but indeed, @chalasr, that would be much more intuitive.

@markusu49

This comment has been minimized.

Show comment
Hide comment
@markusu49

markusu49 Jan 26, 2017

Contributor

How should we handle edge cases then, like passing ThisIsATest to the (lower) camel case converter? It currently converts to _this_is_a_test, but it isn't reversible, as the unit tests make sure that _kevin_dunglas converts to _kevinDunglas.

Contributor

markusu49 commented Jan 26, 2017

How should we handle edge cases then, like passing ThisIsATest to the (lower) camel case converter? It currently converts to _this_is_a_test, but it isn't reversible, as the unit tests make sure that _kevin_dunglas converts to _kevinDunglas.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jan 26, 2017

Member

@markusu49 Your fix is legit on 2.7. If we want to separate, we have to deprecate this argument and introduce the new converter, that must be done on master, in another PR.

Member

chalasr commented Jan 26, 2017

@markusu49 Your fix is legit on 2.7. If we want to separate, we have to deprecate this argument and introduce the new converter, that must be done on master, in another PR.

fabpot added a commit that referenced this issue Feb 16, 2017

bug #21400 [Serializer] fix upper camel case conversion (see #21399) …
…(markusu49)

This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] fix upper camel case conversion (see #21399)

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | ?
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21399
| License       | MIT

Improve upper camel case support of CamelCaseToSnakeCaseConverter (`ThisIsATest` now converts to `this_is_a_test` instead of `_this_is_a_test`).

Commits
-------

81e771c [Serializer] fix upper camel case conversion (see #21399)

@fabpot fabpot closed this Feb 16, 2017

fabpot added a commit that referenced this issue Feb 16, 2017

Merge branch '2.7' into 2.8
* 2.7:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types

fabpot added a commit that referenced this issue Feb 16, 2017

Merge branch '2.8' into 3.2
* 2.8:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types

fabpot added a commit that referenced this issue Feb 16, 2017

Merge branch '3.2'
* 3.2:
  Permit empty suffix on Windows
  fixed CS
  [FrameworkBundle] Remove unused import
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types

@fabpot fabpot referenced this issue Feb 17, 2017

Merged

Release v3.2.4 #21640

This was referenced Mar 6, 2017

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