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

Coding Style Guide #6

Closed
andygrunwald opened this Issue Apr 20, 2017 · 23 comments

Comments

Projects
None yet
7 participants
@andygrunwald
Collaborator

andygrunwald commented Apr 20, 2017

Working in a team on the same codebase might be tricky.
One step to be able to read and understand code of others quite fast is to follow the same coding style guide.
With this you don't have to spent time thinking about the code structure, but can focus on the business logic.

Suggestion

My proposal is to follow a common standard in the php world and use

for your coding style guide. Bigger projects follow those rules as well.

Do we need to check those style guides manually?

No we don't. Because it is a common and well accepted standard, there are tools out there to support this. One example is the PHP_CodeSniffer. The CodeSniffer has predefined rule sets for PSR-1 and PSR-2.
So we are ready to use it directly.

But who formats the complete codebase according the styles? It is a lot of work

Thanks to computers and standards, we don't have to do it.
With the tool PHP Coding Standards Fixer we are able to reformat the code according to the guidelines.

How do we come to a conclusion to solve this ticket?

In my opinion this has to be a decision of the main contributors. I would encourage them to post their +1 for those cgls or a -1 for stay with the current state here.

@jannaahs

This comment has been minimized.

Contributor

jannaahs commented Apr 20, 2017

I had some trouble reading the code at the current state.
In order to become a main contributor I voted with a yes for the Coding Style Guide.

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Apr 20, 2017

In #10 i applied the first PR for module "about".
If @M4LuZ will agree here as well, i will take care about all modules and the rest of the code base in single commits.

But be aware: If you have uncomitted / unmerged changes to the existing codebase / modules, you might get conflicts (because of reformatting).

@JochenJung

This comment has been minimized.

Member

JochenJung commented Apr 20, 2017

I thought the lansuite developer documentation already had the coding style guidelines listed, but the only thing I found was a naming guideline for database tables and fields:

http://lansuite.orgapage.de/index.php?mod=wiki&action=show&name=Database%20Design%20Guideline

andygrunwald added a commit that referenced this issue Apr 23, 2017

andygrunwald added a commit that referenced this issue Apr 23, 2017

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Apr 23, 2017

The pull requests were created via automated tools.
Those automated tools only catch ~80%. But i think it is a good start.

The rest could be done on the way / road when we touch this class.
Do you agree @jannaahs ?

@jannaahs

This comment has been minimized.

Contributor

jannaahs commented Apr 23, 2017

@andygrunwald Can we have a call, please? I have too many questions about being efficent.
Cheers Jan

jannaahs added a commit that referenced this issue Apr 24, 2017

Merge pull request #63 from lansuite/coding-convention-convert-docs
#6: Apply coding style guide PSR 1 and PSR 2 to docs/ folder (with php-cs-fixer)
@byte666

This comment has been minimized.

Collaborator

byte666 commented Apr 25, 2017

@knox : there was a style guide, im pretty sure. But since 2012 we cant find it. ;) => http://lansuite.orgapage.de/index.php?mod=board&action=thread&tid=1333

@byte666

This comment has been minimized.

Collaborator

byte666 commented Apr 25, 2017

What about documenation of the classes? I think thats also a important part of styling guides. In the past I have added many doxygen/phpdocumentor compartible dokumentation. Encapsulate the stuff in classes and documentation was (and is still) a base part to get a stable Lansuite. One of my last actions was to create more classes with clear interfaces and expand the pluginsystem. And there you need a good interface description.

@M4LuZ

This comment has been minimized.

Collaborator

M4LuZ commented Apr 26, 2017

Complete agreement in regard to the code documentation.
But I would suggest to discuss that in a new issue to keep the discussion about the coding style and the documentation requirements separate

@jannaahs

This comment has been minimized.

Contributor

jannaahs commented Apr 26, 2017

@M4LuZ Do you open the issue?

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Apr 26, 2017

@byte666, @jannaahs, @M4LuZ I just created the issue (see #68) and added my 2 cents to this. Feel free to continue the discussion over there.

@ALL: I had a quick call with @jannaahs and he agrees that he will merge most of the CGL changes here. He mentioned that the coding style guide says something about the format of the class names. To rename a class would be a breaking change related to plugins. This is discussed in a other ticket. But i said that those changes were done automatically and the rest of the CGL issues will be fixed on the way while developing. Why? Because it don't add a big benefit / value now to walk through every line of code to reformat, when we touch them anyway later on.

jannaahs added a commit that referenced this issue Apr 26, 2017

Merge pull request #67 from lansuite/issue-6-document-coding-style-guide
Add coding style guide note into the readme (#6)
@M4LuZ

This comment has been minimized.

Collaborator

M4LuZ commented Apr 27, 2017

Fine with that. Certainly have bigger fish to fry ;)

andygrunwald added a commit that referenced this issue May 2, 2017

Merge branch 'master' into issue-68-kill-docs
* master:
  Add coding style guide note into the readme (#6)

andygrunwald added a commit that referenced this issue May 2, 2017

Merge pull request #62 from lansuite/coding-convention-convert-design
#6: Apply coding style guide PSR 1 and PSR 2 to design/ folder (with phpcbf)

andygrunwald added a commit that referenced this issue May 2, 2017

Merge branch 'master' into issue-3-min-php-version
* master: (81 commits)
  Add coding style guide note into the readme (#6)
  formatted css
  #6: Apply coding style guide PSR 1 and PSR 2 to index files (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to index files (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to inc (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to inc (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 wq(with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to  (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to  (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/usrmgr (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/usrmgr (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/troubleticket (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/troubleticket (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/tournament2 (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/tournament2 (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/teamspeak2 (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/teamspeak2 (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/stats (with phpcbf)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/stats (with php-cs-fixer)
  #6: Apply coding style guide PSR 1 and PSR 2 to ./modules/signon (with phpcbf)
  ...
@lxp

This comment has been minimized.

Contributor

lxp commented Jul 14, 2017

TLDR; We need full compliance to our code style right now!

I implemented some automated code style checks with PR #86.
They are currently non-fatal, but to ensure that new PR follow the code style they should be fatal.
This first requires to get the entire codebase to follow the code style.
I understand your concern about breaking existing APIs more often then it might be required.

However, I think we are at a critical point in the whole LanSuite development.
In the long term we need to rewrite a lot parts of the system, but I think we first need to improve the development process itself.
If we don't strictly ensure code style with automated checks right now, we will soon break it again.
I think by breaking the API right now with code style changes and one time again, when we touch individual modules, we might even reduce breaking changes, because for our rewrites we can already be sure to follow our coding style guides.

@M4LuZ

This comment has been minimized.

Collaborator

M4LuZ commented Jul 16, 2017

I see your TLDR and raise you by another one.
If you are too lazy for our thoughts, then I reply in kind ;)

I activated the Travis state as merge requirement for the master branch.
Given that we all already agreed to stick to PSR 1&2, that should force compliance quite nicely, shouldn't it?

@lxp

This comment has been minimized.

Contributor

lxp commented Jul 16, 2017

The TLDR was just meant as subject for my post, I read the whole thread :D

I am fine with most sane style guides, we just need to stick to it, so +1 from my side for PSR1&2.

That's great to make the Travis state a requirement, but we still have the problem that the Travis job itself doesn't fail on PSR1&2 style errors.
This is because our code currently doesn't fully comply with it.
Otherwise, we now would only be able to merge pull requests that first fully fix our codebase :D

My point was that we should just break the "API" for code style right now and change all class and method names to comply with our style.

@BenniGotchi

This comment has been minimized.

Collaborator

BenniGotchi commented Dec 11, 2017

@andygrunwald Did I get this right (from your commit comments), that you used both of the two mentioned tools for the code reformatting?
Was there any meaningful reason for this split usage?
Since @byte666 and me plan to catch up with the current changes and in turn want to integrate the fixes/improvements, we did in the last years, I tried to perform the same reformatting action on our (still svn based) fork. And then I noticed, that neither the phpcbf nor the php-cs-fixer did bring a 100% match over all of the modules.
Only after this experience I noticed your check-in comments here :-/

Can you perhaps summarize shortly, what you did in detail (e.g. which settings / command line parameters used, which module/file done with which tool)

I used the following commands:

php phpcbf.phar --standard=PSR1,PSR2 C:\repos\svn\lansuite_v4\modules
php php-cs-fixer-v2.phar fix C:\repos\svn\lansuite_v4\modules
@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Dec 14, 2017

Did I get this right (from your commit comments), that you used both of the two mentioned tools for the code reformatting?

Yes.

Was there any meaningful reason for this split usage?

I used both to catch all possible cases for an automatic conversion, because it can happen that phpcbf catches cases that php-cs-fixer does not.

Since @byte666 and me plan to catch up with the current changes and in turn want to integrate the fixes/improvements

That is great. Anything we can help here?
Maybe it make sense to first transfer your svn repo to git and then use git cherry-pick. This might make the transfer way easier, because with cherry-pick you can transfer single commits automatically with git.

Can you perhaps summarize shortly, what you did in detail (e.g. which settings / command line parameters used, which module/file done with which tool)

I wrote a small bash script to do this. I did this module by module and folder by folder to get single reviewable changes. This explains the amount of commits. Sadly i don't have the script anymore. I killed it because the job was done. But when i remember i first executed phpcbf on module a. And after this php-cs-fixer on module a. Checked shortly the diff and made a PR.
So i executed all the both apps.

I suggest to chose a module / file that you didn't modified in your fork, test it and compare it with out version of the file. Is there any chance to get access to your svn based fork?
Then i might can have a look if i can reproduce this and help you here with more information.

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Jan 7, 2018

@lxp @BenniGotchi Any news / feedback here? Can we help?

@BenniGotchi

This comment has been minimized.

Collaborator

BenniGotchi commented Feb 5, 2018

Hm, still having trouble with getting the same result as you got.
I tried both

php phpcbf.phar --standard=PSR1,PSR2 C:\repos\svn\lansuite_v4\modules
php php-cs-fixer-v2.phar fix C:\repos\svn\lansuite_v4\modules

and

php php-cs-fixer-v2.phar fix C:\repos\svn\lansuite_v4\modules
php phpcbf.phar --standard=PSR1,PSR2 C:\repos\svn\lansuite_v4\modules

The differences between these two runs are quite little. Mainly spaces or different line splittings for longer statements / function calls with several parameters.

But any of these two conversion runs shows once again formatting differences to your reformatted version. It's mainly 'only' about different indent (number of spaces), but this makes it hard for me, to distinguish formatting differences from real content differences.
And if I change the compare settings, to ignore whitespace differences I can better see the real implementation differences, but I face the whitespace problems once again I want to merge ...

And I'm also wondering, why I get different results than you. Only things I can think of

  • different settings -> can you check my settings again?
  • different versions of the tool

I have

PHP CS Fixer 2.9.0 Speechless by Fabien Potencier and Dariusz Ruminski (454ddbe)
PHP_CodeSniffer version 3.1.1 (stable) by Squiz (http://www.squiz.net)
@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Mar 16, 2018

I had a small test again. We own also a LanSuite instance that had some modifications.
What i did:

Preparing the setup

mkdir php-code-reformat
cd php-code-reformat
composer require squizlabs/php_codesniffer
composer require friendsofphp/php-cs-fixer

After this we had both tools installed.

Modify the old source

Example tournament2 module:

vendor/bin/phpcbf --standard=PSR1,PSR2 /Path/To/LanSuite/modules/tournament2
vendor/bin/php-cs-fixer fix /Path/To/LanSuite/modules/tournament2

Comparing the code

diff -wrq /Source/from/Github/lansuite/modules/tournament2 /My/Modified/lansuite/modules/tournament2

Content like the following shows up

Files /Source/from/Github/lansuite/modules/tournament2/breaks.php and /My/Modified/lansuite/modules/tournament2/breaks.php differ
Files /Source/from/Github/lansuite/modules/tournament2/class_t_league_export.php and /My/Modified/lansuite/modules/tournament2/class_t_league_export.php differ
...

Now lets compare a single file:

diff -w /Source/from/Github/lansuite/modules/tournament2/breaks.php /My/Modified/lansuite/modules/tournament2/breaks.php
7a8
>         include_once('inc/classes/class_masterdelete.php');
25a27
>         include_once('inc/classes/class_masterform.php');

Here you see that the file /My/Modified/lansuite/modules/tournament2/breaks.php has two more includes. The current master branch don't have it anymore, because we switched to autoloading.

The > as first character indicates, that the change is in the second file.
< would indicate that the lines are in the first file.

Does this help a little bit @BenniGotchi ?

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Mar 22, 2018

In general: We agreed on PSR-1 and PSR-2 for LanSuite. This is already documented. See https://github.com/lansuite/lansuite#coding-style-guide

If no counter argument comes in I would close this ticket on 1st of April, because the ticket goal is reached.

@BenniGotchi Any news on your issue?

@andygrunwald

This comment has been minimized.

Collaborator

andygrunwald commented Apr 4, 2018

No new news. Closed. See my last comment.

@M4LuZ M4LuZ added this to the LanSuite 5.0 RC milestone Sep 12, 2018

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