Opened 14 months ago

Closed 13 months ago

Last modified 6 months ago

#3771 closed Task (fixed)

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

Reported by: matt Owned by: matt
Priority: major Milestone: 1.12 - The Great 1.x Backlog
Component: Core Keywords:
Cc: Sensitive: no

Description (last modified by matt)

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.

Change History (11)

comment:1 Changed 14 months ago by vipsoft (robocoder)

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

comment:2 follow-up: Changed 14 months ago by SteveG (sgiehl)

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 ;)

comment:3 Changed 14 months ago by matt (mattab)

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!

comment:4 in reply to: ↑ 2 Changed 14 months ago by matt (mattab)

  • Description modified (diff)

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).

comment:5 Changed 14 months ago by halfdan

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.

comment:6 Changed 14 months ago by vipsoft (robocoder)

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...

comment:7 Changed 14 months ago by matt (mattab)

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 ?).

comment:8 Changed 13 months ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

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

Otherwise, PSR conversion done! https://github.com/piwik/piwik/commit/ae4b03163792f0b6e933933e5d37df87dc3fd566

comment:9 Changed 11 months ago by matt (mattab)

  • Priority changed from normal to major

comment:10 Changed 6 months ago by matt (mattab)

In 1fa8da9b963e99e08c829fe6491e8ccf2d1054e2:

Applying phpstorm code style PSR refs #3771

comment:11 Changed 6 months ago by matt (mattab)

In 84eba85074891bfa6a718ae413a1d87ced4f28ca:

Applying phpstorm code style PSR refs #3771

Note: See TracTickets for help on using tickets.