search

ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 737 forks source link

PSR-2 Coding standard #217

Closed lavoiesl closed 12 years ago

lavoiesl commented 12 years ago

It has come to my attention that this project doesn't respect the PSR-2 Coding Standard.

I am aware that this is not a standard yet, but I still volunteer for a huge code review if you agree.

Cheers

pierrre commented 12 years ago

Do you mean namespaces?

lavoiesl commented 12 years ago

Namespaces are a thing, but this is not obligated as this is a huge BC break. I am more talking about whitespace, visibility declaration, Control Strucutres, etc.

pierrre commented 12 years ago

Kabooom https://github.com/fabpot/PHP-CS-Fixer :D

lavoiesl commented 12 years ago

Yes yes, this is exactly the kind of tool I am talking about.

ruflin commented 12 years ago

I think, first we should should start with PSR-1. As far as I can see, most of this is already in use.

For PSR-2: In general I like the idea, but I only agree 90% on the musts for the coding standard.

Concerning namespace: There is also a ticket to move Elastica to PHP 5.3 where this will be part of.

lavoiesl commented 12 years ago

Cool, we could start with PSR1.

PSR1 should be easy, I can do it right now.

PSR2 is a bit more nasty, I would need to make a list and make sure everyone agrees.

For namespaces, it really isn’t a priority and should only be done if it is needed to convert the codebase to 5.3. A branch should also be kept for < 5.3. Don’t get me wrong, I want it to be converted, I love closures and all, we just need to be careful.

lavoiesl commented 12 years ago

Well, as it turns out, the only remaining issue for PSR1 is the use of camel case for methods, especially starting with an underscore. @ruflin, it is your call for what (if) we do next. We could at least fix the whitespace and advertise such as a coding standard in the README

ruflin commented 12 years ago

I think the discussion already starts here :-) I'm really in favor of tabs for indentation (I know, there is a NOT for that in PSR-1) and I want to keep the the underscore in front of private / protected functions (here we only have a SHOULD NOT). As far as I know, all methods are in camelCase.

The current coding standards are listed here as code sniffer file: https://github.com/ruflin/Elastica/wiki/Coding-guidelines

lavoiesl commented 12 years ago

I used to be in favor of tabs, they have arguments in favor of spaces, but really the point is to follow a standard. And with PHP-CS-Fixer, there is really no work to be done for it to be fixed.

For the underscores in front of methods, it is true it is SHOULD NOT if the method is private/protected. The only I found were in tests, these should be fixed.

So I say, let’s fix all the whitespace, including replacing tabs with spaces, and the remaining camelCase issue. I can start a branch, run PHP-CS-Fixer and we see from there. Do you agree ?