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

Handle lowerCamelCase naming strategy #139

Closed
wants to merge 1 commit into from
Closed

Handle lowerCamelCase naming strategy #139

wants to merge 1 commit into from

Conversation

jry25
Copy link

@jry25 jry25 commented Jun 27, 2012

Hi, camel case naming offers two ways to write camel case :

  • UpperCamelCase, with the first letter on uppercase
  • lowerCamelCase, with the first letter on lowercase

This pull request handle the use of lowerCamelCase naming, by adding a boolean configuration node "lower_camel_case". The use of UpperCamelCase is used by default or if the configuration node "lower_camel_case" is set to "false"

@jry25
Copy link
Author

jry25 commented Jun 27, 2012

@jry25 jry25 closed this Jun 27, 2012
@jry25 jry25 reopened this Jun 27, 2012
@jry25
Copy link
Author

jry25 commented Jun 27, 2012

oops, sorry I didn't want to close this pull request.
I'm a newbie on github, some help will be nice.

@schmittjoh
Copy link
Owner

Do you use upper-case properties?

@jry25
Copy link
Author

jry25 commented Jul 5, 2012

No, I write the properties with lowerCamelCase

@wheelsandcogs
Copy link
Contributor

This would be really useful for APIs that use lowerCamelCase JSON properties.

@schmittjoh
Copy link
Owner

Could you please add/update the tests, and squash your commits?

@wedgybo
Copy link

wedgybo commented Jul 19, 2012

@jry25 Cheers for generating this PR. How are you getting on with the changes @schmittjoh requested? If you need help let us know as I'm keen to get this patch in the core bundle

@jry25
Copy link
Author

jry25 commented Jul 21, 2012

Hi folks,
@schmittjoh and @wedgybo I was extremely busy these times.
I will add/update tests tomorrow.
If I need help I will let you know.
Don't worry, I didn't forget the PR.

@jry25
Copy link
Author

jry25 commented Jul 22, 2012

Hi, I try to figure out where I should update the tests but can't succeed.
CamelCaseNamingStrategy is only use on BaseSerializationTest and PerformanceTest but I don't know where in the code I have to handle lowerCamelCase naming strategy.
Do you know where I should do that ?

@schmittjoh
Copy link
Owner

A unit test is better suited here, I think. Let's create a CamelCaseNamingStrategyTest.

@jry25
Copy link
Author

jry25 commented Jul 22, 2012

Thanks. I'll try to write a unit test. I'll keep you up with CamelCaseNamingStrategyTest
About squash the commits is something like this http://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/ that I have to do ?

@schmittjoh
Copy link
Owner

There are probably more elegant ways to squash, here is what I do:

git reset HEAD~4
git add .
git commit

Replace "4" with the number of commits that you have made so far, and do not add the --hard option to the reset command.

@jry25
Copy link
Author

jry25 commented Jul 22, 2012

Your help is very useful. I did well to ask you some details.

@jry25
Copy link
Author

jry25 commented Jul 22, 2012

I tried to squash the commits but I'm not sure if it properly worked. The log on my local repo doesn't show it and there's no change online

@wedgybo
Copy link

wedgybo commented Jul 23, 2012

git rebase -i 801132b

That commit hash might have to change if there have been any upstream changes since I looked at your repo.

Leave the top commit as pick. Change all the other first words of pick to s or squash in the commit. This will generate a new commit message with all the squashed lines. Delete it all out and put in Adding lower camel case support save then push. Should do the job and end up with a git log like so.

Author: jry25 <jeremy.richard_87@yahoo.fr>
Date:   Wed Jun 27 18:26:57 2012 +0300

Added support for lower camel case

commit 801132b68520b2b1f826a50e4eb12a31e1463303
Author: Johannes <schmittjoh@gmail.com>
Date:   Thu Jun 14 08:50:43 2012 -0500

changed branch alias version

@jry25
Copy link
Author

jry25 commented Jul 23, 2012

I just tried with the method given by @schmittjoh and @wedgybo but as soon as I sync github MacOS application the squash disappear.
I'm very disappointed I'm fluent in svn but apparently not in git.

@stof
Copy link
Contributor

stof commented Jul 23, 2012

well, do the push with the command-line too. The github application may reject force pushes.

@wedgybo
Copy link

wedgybo commented Aug 6, 2012

@jry25 If you open up write access for my account I'm happy to sort the final squash to get this included if you are still having bother with it. Better than copying and generating a new pull request.

@jry25
Copy link
Author

jry25 commented Aug 6, 2012

When I try to push with command line I get this error :

git push
Username for 'https://github.com': jry25
Password for 'https://jry25@github.com':
To https://github.com/jry25/JMSSerializerBundle.git
! [rejected] master -> master (non-fast-forward)
error: failed to push some refs to 'https://github.com/jry25/JMSSerializerBundle.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Do I need to do a git pull ?

@wedgybo
Copy link

wedgybo commented Aug 6, 2012

I'd guess that the squash has changed history so it's no longer a fast-forward commit. As @stof suggested you'll need to use git push --force as you're changing history. Doing this should be fine so long as no one else is working off your master branch and will be effected by the history change.

@jry25
Copy link
Author

jry25 commented Aug 6, 2012

Finally I did it. Thanks you very much for your help. Now everything is in the hands of @schmittjoh

@wedgybo
Copy link

wedgybo commented Aug 6, 2012

No problem :) Cheers for sorting it out. Will hopefully be able to avoid adding in 100s of SerializedName() annotations now.

@alexeyshockov
Copy link

Any progress with issue? This option will be extremely useful, IMO.

@tleidinger
Copy link

Any progress? It refers to this as well #49 doesn't it? It would be super to use lowerCamelCase strategy to deserialize data to Doctrine entity schemes

@schmittjoh
Copy link
Owner

There are still some CS issues, and it would need to be rebased on master.

@poussain
Copy link

Hi,
What are the CS issues ? Can we do something to help ?

$translatedPropertyName = $camelCaseNamingStrategy->translateName($metadata->propertyMetadata['camelCaseProperty']);

$this->assertEquals($translatedPropertyName, 'camel_case_property');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 4 spaces for the indentation, not tabs

@ianunruh
Copy link

Doesn't the term "camel case" generally imply that it starts with a lower-case letter in the first place? Pascal case, on the other hand, implies that it starts with an upper-case letter (see Capitalization Styles on MSDN). The name of this naming strategy is a bit misleading because it isn't ACTUALLY camel case. Not only that (and even more perplexing) is the fact that CamelCaseNamingStrategy also supports SEPARATORS and fully lower-case properties. The way I see it, this class should be split into SeparatorNamingStrategy, CamelCaseNamingStrategy and PascalCaseNamingStrategy. That's just my $0.02 though.

@stof
Copy link
Contributor

stof commented Nov 21, 2012

@iunruh no. camel case can be used for both, which is why we may talk about upper camel case (which is also known as Pascal case) and lower camel case.

@tiger-seo
Copy link

Any progress with issue? This option will be extremely useful, IMO.

@jstoeffler
Copy link

Apparently someone needs to replaces tabs by spaces... Things like this can take several months.
Personally I'm working with @jry25 's fork.

@nifr
Copy link

nifr commented Dec 10, 2012

Exactly what i need right now.

Is the tab to spaces conversion everything left to succeed with this PR ?

@jry25
Copy link
Author

jry25 commented Dec 10, 2012

Hi guys I was totally busy since I created this PR. Hope the commit 93d6a57 will do the job you're looking for. cf https://github.com/jry25/JMSSerializerBundle/commit/93d6a570d18d851c62681fdc53d80f496be77222

@jstoeffler
Copy link

@schmittjoh any comments on the new commit ?

@marcospassos
Copy link

Any news?

@ianunruh
Copy link

You'll probably need to open a PR on the https://github.com/schmittjoh/serializer as well, since the bulk of this component was moved there.

@pietervogelaar
Copy link

I really need this, when will it be implemented?

@marcospassos
Copy link

@schmittjoh there is something else that should be made before this PR get merged?

@ianunruh
Copy link

This pull request might be what most of you are looking for:

schmittjoh/serializer#37

It has already been merged into the master for the actual serializer component. However, the actual configuration change for the bundle is being discussed in this pull request:

#270

@jrmyio
Copy link

jrmyio commented Dec 22, 2013

Waiting for this to be merged. Its kind of sad you need 2+ hours of research for this bundle to just behave camel case (with a lower first letter).

@imunhatep
Copy link

+100 year old issue... common

@gbirke
Copy link

gbirke commented Mar 29, 2014

+1
I'd be very happy if this issue would be resolved.

gbirke added a commit to gbirke/nvcapp1 that referenced this pull request Mar 29, 2014
Ember expects the field names to be CamelCase.
Change serializer naming strategy to reflect that.

See schmittjoh/JMSSerializerBundle#270 and
schmittjoh/JMSSerializerBundle#139
@nifr
Copy link

nifr commented May 5, 2014

ping @schmittjoh

@Raphy
Copy link

Raphy commented Oct 24, 2016

Any news about this PR ?

The solution I've found is to add the parameter in services.yml :

parameters:
    jms_serializer.cache_naming_strategy.class: 'JMS\Serializer\Naming\IdenticalPropertyNamingStrategy'

It works with Symfony 3

Source: gbirke/nvcapp1@3890b28

@goetas
Copy link
Collaborator

goetas commented Dec 19, 2016

Sorry for the terribly long feedback loop

Closing the PR, since too many things changed on master in the last years. If you are still interested in working on this feel free to re-open a new PR based on the current master.

@goetas goetas closed this Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet