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

Formatting - camelCase rather than underscore #108

Closed
wants to merge 1 commit into from

Conversation

crhayes
Copy link
Contributor

@crhayes crhayes commented Mar 22, 2013

How do you feel about having the methods converted to camel case so it is compliant with the PSR-1 spec? (https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md)

If you are open to it I can definitely help with the changes!

@treffynnon
Copy link
Collaborator

If you can think of a way this can be done without breaking backwards
compatibility then I welcome suggestions. How would you consider
implementing this?

Naturally change for no reason is not really beneficial to the current
users of the project. Idiorm and Paris are not members of FIG either so
there is no perceived obligation to amend the API to match any of the PSRs.

Please see my recent blog post for more information:
http://simonholywell.com/post/2013/01/idiorm-and-paris-the-minimalist-orm.html
On Mar 21, 2013 6:07 PM, "Chris Hayes" notifications@github.com wrote:

How do you feel about having the methods converted to camel case so it is
compliant with the PSR-1 spec? (
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md
)

If you are open to it I can definitely help with the changes!


Reply to this email directly or view it on GitHubhttps://github.com//issues/108
.

@crhayes
Copy link
Contributor Author

crhayes commented Mar 22, 2013

This is a simple approach and it's backwards compatible as well. The main advantage here is that users of the library can choose between coding styles, and (as in my case) if their whole project is in camel case the use of this library doesn't have to differ.

What do you think?

@treffynnon
Copy link
Collaborator

This looks great and it is what I was hoping to see. If I was ever to make this change to Idiorm myself this is how I was going to do it. Do you use Paris? These changes will also need to be reflected in Paris as well.

@crhayes
Copy link
Contributor Author

crhayes commented Mar 22, 2013

Awesome, thanks! I will be looking to use Paris as well, so I will fork it and work on those changes over the weekend.

@tag
Copy link
Contributor

tag commented Mar 22, 2013

Great job, straightforward solution (and what I was considering as well, after your initial suggestion). The best part is a future version could switch to camel case for the main definitions, and use underscores as a deprecated backward-compatible option, without any loss of functionality.

@treffynnon
Copy link
Collaborator

a future version could switch to camel case for the main definitions, and use underscores as a deprecated

And @tag , therein lies the beauty and actual eventual aim :)

@ghost ghost assigned treffynnon Mar 25, 2013
@treffynnon
Copy link
Collaborator

@crhayes How have you been getting on with the Paris changes?

@crhayes
Copy link
Contributor Author

crhayes commented Apr 10, 2013

@treffynnon I haven't had a chance to get to them yet, but thanks for the reminder. It's been a crazy couple of weeks with some freelance work and it slipped my mind. Still planning to do it as soon as things ease up.

@treffynnon
Copy link
Collaborator

Thank you for the pull request. Would it be possible to get some tests and update documentation for the functionality, please?

@treffynnon
Copy link
Collaborator

This is still going begging for tests and documentation. Anyone?

@treffynnon
Copy link
Collaborator

Merged in commit 7b1e19f
Documented and tested in commit 7f59e95

@treffynnon treffynnon closed this Aug 28, 2013
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.

3 participants