Coding style policy

[pmaziere]

Indentation is a mess all over the place. Maybe we could start defining some policy regarding this issue and others, so that every one can stick to it.

Concerning indentation, I'm on spaces side rather than tabs. But I guess I could find a way for vim to display the number of columns by indentation level that I like even with tabs. Anyway, I think it really has to be consistent all over the project files.

I don't know much about git hooks, but that could be a good starting point to enforce this policy before integrating a patch. client side example

Any ideas on how to enforce such a policy ?

[aledeg]

To enforce the policy, you have to use tools like https://github.com/squizlabs/PHP_CodeSniffer with some sort of CI like travis. You're out of luck with git hooks because you have to install them manually. Meaning that if you don't want to use them, you can. There might be a way though, if you find a way to add those hooks on github configuration.

[logmanoriginal]

Concerning indentation, I'm on spaces side rather than tabs.

This should be defined in conjunction with a maximum line width, so stuff like this becomes history:

throw new \HttpException('"PHP Simple HTML DOM Parser" library is missing. Get it from http://simplehtmldom.sourceforge.net and place the script "simple_html_dom.php" in '.substr(PATH_VENDOR,4) . '/simplehtmldom/', 500);

It is 220 chars long!

To make the line width predictable I think spaces are the best way to go. Also spaces don't require any special editor setup, so... +1 for spaces I guess :+1:

---

To enforce the policy, you have to use tools like https://github.com/squizlabs/PHP_CodeSniffer with some sort of CI like travis.

@aledeg: Do you, or anyone here, have experience with travis? This would actually help a great deal!

There might be a way though, if you find a way to add those hooks on github configuration.

Is this of any use: https://developer.github.com/v3/repos/hooks/?

[pmaziere]

Is this of any use: https://developer.github.com/v3/repos/hooks/?

I may be wrong but I think it is only for the enterprise version and if it's not it seems it would require a sever somewhere to act on the webhooks notifications

[pmaziere]

I started a page in the wiki I don't know how to handle this for now: could just anyone add up its propositions on the wiki page for each sections ? We could then discuss about each sections here.

[aledeg]

@LogMANOriginal I do have experience with travis. I am using it at work. I only have experience with the enterprise version though. But from my experience, travis documentations are well written.

[pmaziere]

@LogMANOriginal I see you went for 4 spaces indentation… I can't understand why 2 is not enough for most developers: 1 is not enough to be differentiated from the continuation of the line above but what could be the benefit of anything above 2 ? I know it's mostly a matter of personal choices, but could you elaborate on that choice rationale ?

[logmanoriginal]

I do have experience with travis. I am using it at work. I only have experience with the enterprise version though. But from my experience, travis documentations are well written.

Good to know :smile: I'll take a look at Travis. @teromene is working on automated tests with phpunit, so this will become handy.

---

I see you went for 4 spaces indentation…

To make it short: The file was a complete mess and I went with the indentation which was present the most - which turned out to be 4 spaces :smile: Indentation of 2 is fine with me, my editor auto-adapts (to the one used the most).

Which brings me to:

I don't know how to handle this for now: could just anyone add up its propositions on the wiki page for each sections ? We could then discuss about each sections here.

There is not much else we can do until we got something automated up and running. Once we got an automated service like TravisCI we can discuss these things via Issues and PRs. Right now we should focus on stuff that really breaks experience which is: indentation, line length and naming conventions.

I see you have made a proposal for trailing white spaces. I would like to keep that out for now (feel free to use it though), because this is something that doesn't actually troubles us, right?

That being said, naming conventions are already used throughout the core, with some exceptions it's camelCase. Of course class names should be PascalCase (UpperCamelCase). Your proposal goes in that direction (except class names) and I see no technical reason to do it otherwise.

Line length of 80 chars is considered good practice since... ever. There are reasons to go beyond, though there should be a hard-cap at say 120 chars. I like what Linus Torvalds once wrote:

Dammit, code cleanliness is not about "automated and mindless slavish following of rules". A process that is too inflexible is a bad process. I'd much rather have a few 80+ character lines than stupid and unreadable line wrapping just because the line hit 87 characters in length.

Regarding indentation let's just go for 2 spaces. As you said there is no technical reason to have 4 spaces except preferences.

One thing I feel is important: For the core we should enforce these policies (via Travis or such), for bridges these should be guidelines. Since bridges are not part of the core this is acceptable in my opinion.

[aledeg]

with phpcs, you can decide what you can enforce as a coding style. There is some profiles with different sets of rules. You can even create your own ruleset if needed.

[pmaziere]

@LogMANOriginal ok with everything you said

[logmanoriginal]

with phpcs, you can decide what you can enforce as a coding style. There is some profiles with different sets of rules. You can even create your own ruleset if needed.

I think I got you now, thanks again :laughing: I had a quick look at both and they actually provide good templates here and here to work with. I'll do some testing, maybe we get something running in a few days.

[aledeg]

Here's an example of what I've set at work. I cannot share much but I think it could give you an insight of what is possible.

phpcs configuration:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="Project custom standard">
    <description>Current project custom standard</description>

    <exclude-pattern>./vendor</exclude-pattern>
    <exclude-pattern>./web</exclude-pattern>
    <exclude-pattern>./var</exclude-pattern>

    <rule ref="PSR2"/>

    <rule ref="PSR1.Classes.ClassDeclaration.MissingNamespace">
        <exclude-pattern>./tests/features/bootstrap/FeatureContext.php</exclude-pattern>
        <exclude-pattern>./app/AppKernel.php</exclude-pattern>
        <exclude-pattern>./app/AppCache.php</exclude-pattern>
    </rule>
</ruleset>

travis configuration:

...
script:
  - vendor/bin/phpunit
  - vendor/bin/phpcs . --standard=phpcs.ruleset.xml --warning-severity=0 --extensions=php -p
  - vendor/bin/behat -fprogress -v
...

[logmanoriginal]

Thanks to @aledeg I found a way to introduce TravisCI with PHP_CodeSniffer. Not sure if you have access, but here is what the results look like. https://travis-ci.org/LogMANOriginal/rss-bridge

Based on this branch: https://github.com/LogMANOriginal/rss-bridge/tree/ContinuousIntegration

I'll push these changes directly to master in a few minutes, but one of the admins needs to authorize Travis to access RSS-Bridge. Which I request next. Stay tuned :blush:

Merged: https://github.com/RSS-Bridge/rss-bridge/commit/accbe8c06f0c56f186a32dcc323916fae24cfb5d, authorization for Travis pending...

[aledeg]

Good job. Now there is a lot of corrections to add :) Good thing that phpcs comes with phpcbf that corrects most of the errors.

[Frenzie]

Boo spaces. Go tabs! :-P

I always display my tabs as two spaces though, so there's that.

[logmanoriginal]

Good job. Now there is a lot of corrections to add :)

I have already prepared, most can be solved via simple regex search & replace operations, which is what I'll push once Travis is online... :smile:

Good thing that phpcs comes with phpcbf that corrects most of the errors.

That is actually an amazing feature, which left me :open_mouth: the first time I read about it :smile: The reason why I didn't add it (yet) is, because it adds a lot of noise (commits). I think we should wait how things work out and if it is too much work fixing the chaos, we can activate automatic corrections later. (again, the first cleanup can easily be done like I mentioned above)

Boo spaces. Go tabs! :-P I always display my tabs as two spaces though, so there's that.

I'm with you :cry: :wink:

The reason for spaces is to avoid problems with editors and make the line width more calculable. Another reason is because @pmaziere was first :laughing:

My editor auto-adapts, so I don't really care much. Also I can easily convert between tabs/spaces (this could also be automated on load/save) :innocent:

Source: https://github.com/Microsoft/vscode/issues/1228#issuecomment-164426832

It might be possible to do that directly via git, though I have never tried: http://stackoverflow.com/a/2318063

[logmanoriginal]

@mitsukarenai / @ArthurHoaro: Did one of you receive a notification from Travis regarding the access request? This requires an admin of the RSS-Bridge organization. Would be great if we could get this running soon :grinning:

I'm not entirely sure how this is setup with organizations, though it is possible:

[teromene]

I am more with tabs as well, as it allows everyone to display the code the way they prefer. As for the line size, I think that phpcs makes the calculation automatically.

[pmaziere]

if the majority goes for tabs let's go for tabs

[teromene]

@LogMANOriginal : We can't set up Travis, as we are not Owners of the organization, but only members. This requires @mitsukarenai or @ArthurHoaro

[logmanoriginal]

I think that phpcs makes the calculation automatically.

Yes it does

@LogMANOriginal : We can't set up Travis, as we are not Owners of the organization, but only members. This requires @mitsukarenai or @ArthurHoaro

Yes, I know. However we are able to select the repository in our Travis accounts and request access, which notifies the admins. I did that, so let's wait for the things to come.

[aledeg]

@LogMANOriginal I've checked the travis configuration you wrote. You check the code against different PHP versions. I don't know if it's important that it works on all versions. For instance, you could introduce a matrix to allow some failures on versions that are not important.

For example:

matrix:
  fast_finish: true
  allow_failures:
    - php: nightly
    - php: hhvm

It will allow to fail some tests without failing the whole suite. For now, it is not really important as you only test for coding standard. But in the future, if you want to include some unit tests and/or test scenarii, it could be useful.

[logmanoriginal]

You check the code against different PHP versions. I don't know if it's important that it works on all versions.

I started with this and never changed it later. Right now we only test coding standards, so PHP 5.4 should be enough.

I really have to find some time to read their documentation (It's very well written btw :+1:), but I'm very busy for the rest of this week, so I'll have to do that on the weekend.

But in the future, if you want to include some unit tests and/or test scenarii, it could be useful.

Check out #379

[teromene]

I think we should test on all the supported version of PHP, as our users may be using any of these versions.

[aledeg]

@teromene For unit test for sure. But as mentioned @LogMANOriginal, for coding standard, you could test only on one version.

[logmanoriginal]

I think we should test on all the supported version of PHP, as our users may be using any of these versions.

Hmm, you are right, it seems phpcs can detect deprecated functions: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Generic/Sniffs/PHP/DeprecatedFunctionsSniff.php

This means we have to run it on multiple versions to get notified... Know what, the current script is working, so let's keep it as it is. We can add the idea from @aledeg to allow failures of nightly and hhvm.

[logmanoriginal]

@aledeg Please correct me if I'm wrong:

---

phpcs can detect deprecated functions (there is even this project for extension :open_mouth:). Since .travis.yml supports conditional statements and phpcs allows for multiple standards, we can check coding standards on one particular PHP version (5.4) and check for deprecated functions on all. Of course 'nightly' and 'hhvm' may fail if they so desire.

---

Of course this Issue #362 is about the coding standard, so for that particular requirement only one PHP version is necessary. We can all agree indentation isn't going to break in future versions :smiling_imp:

[aledeg]

I wasn't aware of that extension. I will report that at work, it can become handy. I never had to use it, because I code only on PHP 7.

My guess is that you have to run it on any version of PHP and configure it to check the supported versions.

We can all agree indentation isn't going to break in future versions :smiling_imp:

You'll never know ^^

[logmanoriginal]

I have just applied some coding styles and updated the phpcs checks, such that travis will at least pass (once we got it running that is -- hint @mitsukarenai :grinning:)

For now I decided to go with:

• Tab indentation
• No whitespace at end of lines or before semi-colon
• Line width should be <80, absolute max is 120
• Spaces must be after around comma or assignment operators

Function names and such are still as before. However the phpcs.xml defines some additional checks, so have a look if you are interested.

I did this in order to get at least ANY style into the core framework, which was a complete mess before :rage:

Please have a look and if you find anything absolutely not acceptable let's discuss it here. If there are no objections, I'll begin writing the phpcs configuration as rules into the Wiki after a few days/weeks (depends on my free time :watch:)

Oh, yeah, here is the passing travis on my branch: https://travis-ci.org/LogMANOriginal/rss-bridge/builds/159008586 --- It's so green :heart:

[mitsukarenai]

wakes up uuh, getting Travis running ? ...who even is Travis ? :sweat_smile: I'm a bit like "as long the code works", but better code design allows better visibility, thus easier human review and contributions. So yep !

[teromene]

@mitsukarenai You should have received a email concerning the setup of Travis, used to test and enforce, between others, the style checks. As, with @LogManOriginal, we are not administrators of the organisation we can't do it...

[logmanoriginal]

uuh, getting Travis running ? ...who even is Travis ? :sweat_smile:

Travis-CI allows to perform automated checks on source code from GitHub. It basically is a server running lots of virtual machines that clone your repository and perform checks based on what you configure in your travis.yml file.

Here is an live example of Travis doing checks on PRs, which actually gives you a nice indicator and some feedback on what actually went wrong: https://github.com/travis-ci-examples/php/pulls

Last but not least we can then add another banner to the readme file to indicate the working state of the master branch like this:

Not sure if you received the notification or not, but in order to grant access for travis to RSS-Bridge you'll have to login at https://travis-ci.org/ with your GitHub account and grant access to the RSS-Bridge repostory via the menus. It's basically turning a switch like this under your "organization" settings:

[Frenzie]

We use Travis in KOReader. It can be quite a useful way to prevent regressions.

[ArthurHoaro]

Shaarli uses Travis to run its unit tests, if you want another PHP example. Setting up Travis isn't really difficult, writing unit tests can take a bit longer. :smiley:

[teromene]

@ArthurHoaro : You have admin access to rss-bridge, but we haven't. We just need you or @mitsukarenai to flip the switch in Travis interface :smile:

[ArthurHoaro]

Done!

[teromene]

I have created a first base of a Contributing file here. Please tell me what you think.

[Frenzie]

I assume "squash your commits" isn't meant to imply one issue, one commit but merely to squash, e.g., minor typo fix commits? Because I think it's much easier to dissect or follow along with multiple smaller commits. What I mean is illustrated quite nicely in #341. I don't think that would be improved either by squashing or by splitting it up into multiple PRs.

It could also be useful to give some guidelines about how to write the commit message. I've personally used Bridge: issue (cf. #368) but I've since noticed that the de facto standard for RSS-Bridge seems to be [Bridge] issue (cf. #348) and category: issue.

[teromene]

Yes, that's what I mean when I say "squash the commits". I have updated & extended the file.

[teromene]

Updated again using @LogMANOriginal remarks.

[logmanoriginal]

Okay, this has been sitting around for quite a while now. I'm fine with the proposed styles and I don't think it gets any better by ignoring it any longer :grin:

I just compared the current phpcs.xml against the definitions in the contributions.md. The current version works if we add Generic.NamingConventions.CamelCapsFunctionName (for camelCase function names) and Zend.NamingConventions.ValidVariableName (for camelCase variable and function names). This can be applied also to the core API for consistency.

@teromene For the contributions.md please consider my last comment.

I'll create a PR to add the contributions.md, update phpcs.xml and fix everything. That way we don't break master and we can squash and merge everything in one go.

[logmanoriginal]

Okay, the PR passed Travis :tada:

Please have a look at #475 and let me know what you think. Especially about forcibly camel-casing function names.

In my opinion we should allow (partial) upper-case names where it makes sense (like 'RSS' instead of 'Rss'). Unfortunately I couldn't find a sniff that allows partial upper-cases, so yeah...

If there are no objections I'll merge someday next week, before master diverges :fearful:

[logmanoriginal]

https://github.com/RSS-Bridge/rss-bridge/commit/761c66d81383510625b28a3fc6f5c3b73be4480f happened. The only thing missing is the CONTRIBUTIONS.md

[teromene]

CONTRIBUTING.md has been added in 2dda74dfe722e29b19bdc95b63bb44cdf3853cb2.