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

Feature: maintain coding style using PHP Code Sniffer #469

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

koenpunt
Copy link
Collaborator

@koenpunt koenpunt commented Dec 9, 2014

Added PHP Code Sniffer as composer dependency and to Travis config.

This is probably the time to switch to more sensible defaults:
tabs or spaces
OpeningFunctionBraceBsdAllman or OpeningFunctionBraceKernighanRitchie

omit use of call_user_func_array

remove error suppression
@Rican7
Copy link
Collaborator

Rican7 commented Dec 10, 2014

It would be ideal to conform to PSR-2... but that would be a huge BC break, since it would require changing the name of every single method. Lol.

In the meantime, its still better to ensure that we maintain the somewhat strange style of this project than have mixed YOLO styles. :P

@Rican7 Rican7 added the feature label Dec 10, 2014
@Rican7
Copy link
Collaborator

Rican7 commented Dec 10, 2014

Here's a convenient review link to review the changes while ignoring whitespace:
https://github.com/jpfuentes2/php-activerecord/pull/469/files?w=1

@Rican7
Copy link
Collaborator

Rican7 commented Dec 10, 2014

This looks good to me 👍

@koenpunt
Copy link
Collaborator Author

Does PSR requires a coding style for method names? I'd though it was only for project/class structures. But I did not read into it at all, so don't know..

@Rican7
Copy link
Collaborator

Rican7 commented Dec 10, 2014

Yea. PSR-2 is an extension of PSR-1, which requires method names to be camel-cased:

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md#43-methods

@cvanschalkwijk
Copy link
Collaborator

PSR-2 will need to wait until a major point release where BC will not be an issue. As for tabs and spaces, let's go with 4 spaces. I believe @jpfuentes2 is on board with that as well.

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

Successfully merging this pull request may close these issues.

3 participants