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

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

Closed
mattab opened this issue Feb 22, 2013 · 11 comments
Closed
Assignees
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Feb 22, 2013

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
Copy link
Contributor

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

@sgiehl
Copy link
Member

sgiehl commented Feb 22, 2013

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
Copy link
Member Author

mattab commented Feb 22, 2013

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
Copy link
Member Author

mattab commented Feb 22, 2013

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
Copy link
Member

halfdan commented Feb 23, 2013

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
Copy link
Contributor

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
Copy link
Member Author

mattab commented Feb 27, 2013

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
Copy link
Member Author

mattab commented Apr 4, 2013

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

Otherwise, PSR conversion done! ae4b031

@mattab
Copy link
Member Author

mattab commented Oct 8, 2013

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

@mattab
Copy link
Member Author

mattab commented Oct 8, 2013

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

@mattab mattab added this to the 1.12 - The Great 1.x Backlog milestone Jul 8, 2014
@mattab mattab self-assigned this Jul 8, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
@mattab
Copy link
Member Author

mattab commented Jan 18, 2016

See also our follow up issue Automatically convert our code base to use a consistent code style #9545

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

4 participants