Coding guidelines: chose an indentation style

[tuxayo]

There are currently some inconsistencies leading to indentation issues between GitHub's code view and the different text editors out there.

We should choose one style and add it to the guidelines so we will gradually unify the indentation style as when new code will get written, it will fix the existing inconsistencies.

After few minutes jumping through the 70 000 lines of PHP code of Coral, I've found that > 90% of the indentation are tabs.

So tabs would be the simplest choice to avoid a mass conversion. In the case where tabs are chosen, the length should be chosen to preserve alignment (for example in a function with many params on multiple lines)

[tuxayo]

Just an example of an alignment that rely on tab length (looks good with tabs of length 2) and another one that seems to be made for tabs of length 4.

[t4k]

I’m not sure we need to define the length of tabs; individuals can do that how they like in their editors. If we do, we should add it to .editorconfig as proposed in #208.

If there is consensus around tabs, I’ll make the bulk change in a PR.

I feel like there needs to be a reference to the Silicon Valley episode in here somewhere, but I am failing to come up with something clever.

[PaulPoulain]

t4k: that's a troll, and you jumped in ;-)
we have to chose either use TAB or SPACE for indentation. The mix is a mistake.
There are a lot of arguments against tab. One of them being that there can be a variable number of them depending on how you display them.
That's what tuxayo wrote last july.
With tab=2 spaces, you can be putting more tabs for the same indentation.
For example:

if ($bla="blabla") {
>do_this;      #> (is a tab)
    do_that;   #with 4 white spaces
}

If you have tab=4 spaces, you'll see things properly aligned. if you have tabs=2 spaces, you'll think there's a missing tab. So you put 2, and ppl using tab= 4 spaces will think that's wrong.
So ==> use tab or spaces, but don't mix.

Note that I personally prefer, from far, spaces. But don't jump into the troll. If 90% of the code has tabs, let's use 100% tab ;-)

[remocrevo]

I agree with Paul. I prefer spaces, but it seems the simplest solution for the CORAL code is tabs.

[jeffnm]

I think the biggest thing is to be consistent. I like Tommy's idea to use .editorconfig to specify these settings.

[t4k]

Just to be clear, I’m personally in favor of 2-spaces indentation as well, coming from a Drupal background where we have clear code-style guidelines (https://www.drupal.org/docs/develop/standards/coding-standards). I guess there isn’t a consensus yet, but since the codebase already has vast majority of the code with tabs, it seemed that would be the least disruptive change.

On the other hand, it would just be one commit if we were to convert to spaces, but it is likely that every outstanding pull request would have to be redone in order to conform to the style guidelines.

I’m happy to do the update still and put in a pull request if we reach consensus.

[tuxayo]

I’m not sure we need to define the length of tabs; individuals can do that how they like in their editors.

If we want to do stuff like mentioned in the 30 Jun 2016 comment, we need everyone to have the same tab length. So we see the same things wish enables to use tab length dependent alignments.

[t4k]

If we want to do stuff like mentioned in the 30 Jun 2016 comment, we need everyone to have the same tab length. So we see the same things wish enables to use tab length dependent alignments.

Is there a way to define the tab length inside a text file? I am not aware of that. Tab length is a setting in whatever viewer/editor one is using as far as I know. The only way I understand to kind of make it universal for the project is by using the .editorconfig file that we have now included, but it is not guaranteed because anyone can open the files in any viewer/editor that defines tabs however it wants.

This is one of the arguments for using spaces, I believe. There is no unknown factor in the length of tabs…

[t4k]

I’d like to start cleaning up some of the CORAL files as I am working on them. I’ll update the https://github.com/Coral-erm/Coral/wiki/Coding-Guidelines when we come to a consensus on things or if no one chimes in and I just start applying something. I will document my choice and changes we can always revisit anything.

Below are some examples of coding standards that I will pick and choose from for ours. Please chime in if you have preferences.

• https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
• https://php7.org/guidelines/pgs-2.html
• https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP
• https://www.drupal.org/docs/develop/standards

[tuxayo]

I’d like to start cleaning up some of the CORAL files as I am working on them. I’ll update the https://github.com/Coral-erm/Coral/wiki/Coding-Guidelines when we come to a consensus on things or if no one chimes in and I just start applying something. I will document my choice and changes we can always revisit anything.

+100 for this strategy which will allow to move forward to a cleaner codebase.

[t4k]

I found an interesting report: https://squizlabs.github.io/PHP_CodeSniffer/analysis/

Part of the need for a coding standard is so that we can all use the same one in our IDEs and it will be enforced and we can run reports on existing code with something like PHP_CodeSniffer.

Initially I was leaning towards using WordPress coding standards for consistency, but then I started looking more closely at some of the de facto standards in the CORAL codebase. We’d have problems with all of our class names if we adopted that standard: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

Because of that drawback, I am now leaning toward using the Drupal coding standards, which would allow us to keep our UpperCamelCase class names. We’d have to address function names, but we’d have to do that with WordPress as well. All the lowerCamelCase methods will be correct though.

Any opinions? What else should I be considering when we need to apply standards to our codebase?

[techsvcslib]

I’m ok with Drupal coding standards but I leave it to those who do coding to speak the loudest on this topic. Steve On Aug 23, 2017, at 7:27 PM, Tommy Keswick <notifications@github.com<mailto:notifications@github.com>> wrote: I found an interesting report: https://squizlabs.github.io/PHP_CodeSniffer/analysis/ Part of the need for a coding standard is so that we can all use the same one in our IDEs and it will be enforced and we can run reports on existing code with something like PHP_CodeSniffer. Initially I was leaning towards using WordPress coding standards for consistency, but then I started looking more closely at some of the de facto standards in the CORAL codebase. We’d have problems with all of our class names if we adopted that standard: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions Because of that drawback, I am now leaning toward using the Drupal coding standards, which would allow us to keep our UpperCamelCase class names. We’d have to address function names, but we’d have to do that with WordPress as well. All the lowerCamelCase methods will be correct though. Any opinions? What else should I be considering when we need to apply standards to our codebase? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#55 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AC9mt_iIy6ityYTgvoLZGfsMXH2L3UEBks5sbMOFgaJpZM4JAMBT>.

[tuxayo]

Initially I was leaning towards using WordPress coding standards for consistency, but then I started looking more closely at some of the de facto standards in the CORAL codebase. We’d have problems with all of our class names if we adopted that standard: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

Because of that drawback, I am now leaning toward using the Drupal coding standards, which would allow us to keep our UpperCamelCase class names. We’d have to address function names, but we’d have to do that with WordPress as well.

We could take the nearest existing standard and bend it match our most import de facto standards. And the linter config (for example the PHP_CodeSniffer config) could be left in the repo.

[t4k]

We could take the nearest existing standard and bend it match our most import de facto standards. And the linter config (for example the PHP_CodeSniffer config) could be left in the repo.

That’s what I’ve been trying to analyze manually. Does anyone know any important de facto standards in the CORAL code? In terms of naming things, particularly variables, there doesn’t seem to be many. We’d have to adapt the code to a standard.

If we don’t go completely with an existing standard, my concern is that no one will have the time to write tests for a linter like PHP_CodeSniffer. I think we’d be better off making code conform to something existing rather than maintaining tests for our own coding standards as well as the code itself.

[tuxayo]

Does anyone know any important de facto standards in the CORAL code?

After digging in issues, the only remotely relevant result is #102

If we don’t go completely with an existing standard, my concern is that no one will have the time to write tests for a linter like PHP_CodeSniffer. I think we’d be better off making code conform to something existing rather than maintaining tests for our own coding standards as well as the code itself.

What do you mean by tests for a linter? It would be taking a like .editorconfig that represents an existing standard and change a part of it.

It seems way quicker that formatting all the codebase. (is it practically doable? because yes that would be the best solution in terms of results)

We might have to apply the standard only to the new and modified code to make the codebase slowly converge. (in the case nobody can afford such a big reformatting work)

[t4k]

Beyond an .editorconfig file I mean the rulesets used for PHP_Codesniffer. See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards and https://www.drupal.org/project/coder for the examples of sniffs.

An .editorconfig file cannot check things like:

function funstuff_system($field) {

vs.

function funstuff_system ($field)
{

To check them or to enforce them functions need to be written like https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/Functions/FunctionRestrictionsSniff.php or http://cgit.drupalcode.org/coder/tree/coder_sniffer/Drupal/Sniffs/Functions/FunctionDeclarationSniff.php and I don’t think we want to be maintaining our own versions of these things.

We don’t have to format the whole codebase at once, we could just enforce the standards on any new files or modified files as the pull requests come in.

Again, this would be used in addition to an .editorconfig file.

[jcuenod]

I definitely think it's better to use a standard than modify one. I also think it's a good idea to use one that's widely adopted so wordpress appeals to me for that reason - but Drupal et. al. are going to be just as well supported for our purposes I think (perhaps better since WP is often catering to a less skilled coding base).

I want to say it's reasonable to enforce the standard only on pull requests. It can be an issue that slowly gets worked through the code base. The problem is that if the standard has anything that affects class/function names we will have to edit other files and suddenly a simple PR explodes.

With regards to tabs/spaces: I think we should do 3 spaces per indent and that's a hill I'm willing to die on!

[t4k]

Since there hasn’t been input here in over a month and we have a general consensus on choosing an existing coding standard, I am going to move forward in adopting the Drupal Coding Standards https://www.drupal.org/docs/develop/standards for CORAL.

The reason for this decision is that with WordPress vs. Drupal, Drupal has more flexible naming conventions for variables and class naming is more compatible with the existing CORAL codebase, as far as I can tell.

Drupal Naming Conventions https://www.drupal.org/docs/develop/standards/coding-standards#naming :

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.

Drupal OO Naming Conventions https://www.drupal.org/docs/develop/coding-standards/object-oriented-code#naming :

Classes and interfaces should use UpperCamel naming. [etc.]

With WordPress we would have had to change many things to conform to their naming conventions https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions :

Use lowercase letters in variable, action/filter, and function names (never camelCase). Separate words via underscores. Don’t abbreviate variable names unnecessarily; let the code be unambiguous and self-documenting.

and

Class names should use capitalized words separated by underscores. Any acronyms should be all upper case.

I understand that CORAL code will not be immediately compatible with the Drupal Coding Standards, but working together over time we will clean up existing and contribute more standardized code to make it easier on everyone.

We’ll need documentation for developers in how to set up PHP_CodeSniffer in their editors of choice.

The first step will be to change the .editorconfig file.

If we find any systematic problems with this in the future, we should open new issues and discuss. Once the new .editorconfig file is merged, I believe we can consider this issue closed.

[tuxayo]

Yay, this issue can be closed :)

Should we gradually insist that new patches comply with these guidelines as a condition to be merged? (that would be the beginning of enforcing a definition of done)

[jcuenod]

I feel like a problem would be the rest of the file - does the unpatched part get updated? If so, that's going to make debugging commits (and blaming) difficult.

[PaulPoulain]

@jcuenod I fear that it's a neverending debate. We have to chose our poison, there's no unharmful option. In the Koha project, we faced the same problem. We favored git history/blame, so there's some things that are still not respecting coding guidelines in the code.
The approximative rule is: if you change something that is referring to some non-valid coding, then fix it. If you change something in a file that contains something invalid, but not related to invalidity, no change.

[t4k]

This note was on one of the Coding Standards pages I found and I think we should heed it for our project:

Note: Do not squeeze coding standards updates/clean-ups into otherwise unrelated patches. Only touch code lines that are actually relevant. To update existing code for the current standards, always create separate and dedicated issues and patches.

[jcuenod]

Might be worth noting this: https://www.drupal.org/node/1419988
(and https://github.com/squizlabs/PHP_CodeSniffer)
for dynamically checking our own code.