Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Automatically reformat all source code in Piwik to follow updated coding standard #3771

Closed
mattab opened this Issue · 10 comments

4 participants

@mattab
Owner

This task goal is to cleanup a bit since as the team has grown there has been some freedom in the style, but it's nicer for everyone to see/read one style.

This ticket is multi task:

  • Setup perfect "reformat code" setup in PHPStorm (the best PHP IDE on planet earth), and record this profile under misc/ directory.
  • Write documentation on perfect PHPStorm setup for Piwik devs (inc. using the reformat profile)
  • Apply the profile to the full source code
    • PHP files
    • JS files
    • templates?
  • Review PHP and JS Coding Standards and update them with new knowledge from this 4 year old guide (eg. to fix: comments are not always must have, often much better to write small methods with clear names.).
  • Create a unit test to fail if a file is submitted with 4 spaces instead of tabs (so dev remembers to configure her IDE).

I've got the profile in phpstorm and will commit all updated PHP files soon.

Any suggestion on how to improve the code & standards is always welcome.

@robocoder
Collaborator

For PHP coding standards, I'm leaning towards PSR0,1,2.

@sgiehl
Collaborator

Please do not automaticly update all php files to match any coding standards. That will cause merging problems for everyone having any unpushed changes to those files. And I have quite a lot of those in my refactoring branch...

PSR 0/1/2 sounds good. It uses spaces instead of tabs. I don't like tabs ;)

@mattab
Owner

the big difference is the curly braces on the same line, isn't it?

I guess it's a difference but Im still not keen to change this style, until the code is much clearer. Right now it would look messy if we changed to this style. Id like to refactor existing code and refactor a lot at some point!

@mattab
Owner

Replying to SteveG:

Please do not automaticly update all php files to match any coding standards. That will cause merging problems for everyone having any unpushed changes to those files. And I have quite a lot of those in my refactoring branch...

PSR 0/1/2 sounds good. It uses spaces instead of tabs. I don't like tabs ;)

thanks for the reminder... We will specifically wait for your branch (as I believe you're the only one to have a big change in the works). looking forward to it!

But we have to apply the changes automatically at some point, since it's time consuming to have spaces & style differences in the files. I think we'll keep tabs, since it has the advantage that you can change the width in IDE & it's what we're using in 95% of the code...

Adding to ticket:

  • Create a unit test to fail if a file is submitted with 4 spaces instead of tabs (so dev remembers to configure her IDE).
@halfdan
Collaborator

PSR-0/1/2 really sound great - yet I agree with matt that we should keep tabs instead of spaces.

@matt, for the automatic reformatting it might be interesting to use https://github.com/fabpot/PHP-CS-Fixer - it applies PSR-0/1/2 automatically to all files in a directory (you can deactivate the indentation fix). Could be easier than doing it with PhpStorm.

@robocoder
Collaborator

Cs-fixer uses regexes, not the tokenizer, so it sometimes makes mistakes. We would need to manually verify its changes.

I don't mind the switch from tabs to spaces. There's no visual difference because we used ts=4, and I don't think the additional storage requirements will be noticed by users. Plus, iirc GitHub (or is it gitlab?) uses ts=2 in diffs, so it's distracting when tabs and spaces are both used for indentation. So +1 for eliminating tabs.

Agree with not bulk updating all the files (at least not in master). Need a plan...

@mattab
Owner

I changed my mind, we should use full PSR 0/1/2 including white spaces instead of tabs (it seems more people like spaces these days).

Also I'm hoping to use phpstorm rather than another tool, hope it will work.

I plan to make this change some time in March (after 1.11 before 1.12 ?).

@mattab
Owner

I didn't add the unit test as it would be annoying.

Otherwise, PSR conversion done! ae4b031

@mattab
Owner

In 1fa8da9: Applying phpstorm code style PSR refs #3771

@mattab
Owner

In 84eba85: Applying phpstorm code style PSR refs #3771

@mattab mattab added this to the 1.12 - The Great 1.x Backlog milestone
@mattab mattab self-assigned this
@sabl0r sabl0r referenced this issue from a commit in sabl0r/piwik
@mattab mattab Applying phpstorm code style PSR refs #3771 1fa8da9
@sabl0r sabl0r referenced this issue from a commit in sabl0r/piwik
@mattab mattab Applying phpstorm code style PSR refs #3771 84eba85
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.