Code reformat PSR2

[sergey-rud]

Just suggestion: I think It would be nice and more cleaner if the code will be formatted using PSR2.

[brianjmiller]

This is unlikely to occur as the code is relatively stable and while not necessarily PSR2 it is at least consistent, and there wouldn't be any real benefit in taking this on, and it would require a ton of testing which I don't think our coverage tolerates at this point. Will consider PRs but it would take a fair amount of convincing and that PR would need to be accompanied (or preceded) by some serious testing coverage increase.

[WillSkates]

@brianjmiller The real benefit to taking this on is giving people a style model that they can follow. Even though the code is consistent we have to infer what the style is instead of being told what the style is. This only increases the margin to which someone could get it wrong.

Beyond that if the guidelines were laid out then they wouldn't be consistent with those that people generally try to follow (being PSR1 & PSR2) so you give more for people to learn in order to work on your product.

It seems like using PSR1 & PSR2 and having an automated tool such as StyleCI check your PR's for you will fit fine.

[brianjmiller]

@WillSkates oh, I know the reasons trust me, and that is a great suggestion for a non-distributed library or one with excellent test coverage, unfortunately I feel like that ship has sailed as the library is widely distributed and doesn't have good enough test coverage to adequately test a sweeping style change. For the size edits that are occurring it really shouldn't be difficult to look at the rest of a file and conform, and the potential for instability isn't worth the small gain offered by switching to a different style, particularly since we're already consistent. If people want to contribute the best thing to do is to improve the test coverage.

[WillSkates]

@brianjmiller There is no sweeping (i.e. wide range or amount) style change provided by PSR2 that would affect your downstream users.

The only way that PSR2 would affect this is by changing the names of your classes and/or methods that are publicly visible.

In PSR2 could change the ways that the names of things are written in two ways:

1. They must all use camel casing.
2. No names should be prefixed to indicate their visibility.

The problems with upgrading the publicly visible interface to PSR2 come in where the lib is not consistent:

FILE: /usr/src/src/Agent.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 139 | ERROR | Method name "Agent::setMbox_sha1sum" is not in camel
     |       | caps format
 140 | ERROR | Method name "Agent::getMbox_sha1sum" is not in camel
     |       | caps format
----------------------------------------------------------------------

FILE: /usr/src/src/Verb.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 56 | ERROR | Method name "Verb::Voided" is not in camel caps format
----------------------------------------------------------------------

FILE: /usr/src/src/Person.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 76 | ERROR | Method name "Person::setMbox_sha1sum" is not in camel
    |       | caps format
 77 | ERROR | Method name "Person::getMbox_sha1sum" is not in camel
    |       | caps format
----------------------------------------------------------------------

It should be fine to move to PSR2 for the next major version bump you stated. We can also aim for 100% code coverage as well.

[brianjmiller]

The problems I'm concerned about aren't the public interface changes, it is the sheer amount of change and the potential for human errors to happen that destabilize the lib, which begs the question of whether or not someone has done it. If it is a small change to a couple files then that's one thing, but if we're talking about adjusting whitespace handling around every operator in every function in every file (which is intended as a "style change" example) then I'm just not interested as it just isn't worth the risk.

[WillSkates]

@brianjmiller I know. I didn't think that their would be anything that could cause significant issue. That was my intuition however so I went back and checked.

Essentially what we are looking for is lines of code that cant be fixed automatically (either named incorrectly or longer than 120 characters) and aren't covered in the test suite.

By my count there are two lines (2/1429): https://github.com/RusticiSoftware/TinCanPHP/blob/master/src/ContextActivities.php#L70 https://github.com/RusticiSoftware/TinCanPHP/blob/master/src/Statement.php#L246

In order to reach that conclusion I.....

1. Ran phpcbf over all of the code in the source directory (phpcbf --standard=PSR2 ./src).
2. Ran phpcs to check for any remaining things that couldn't be fixed (phpcs --standard=PSR2 ./src).
3. Ran the test suite and asked it to generate a html cc report (vendor/bin/phpunit --coverage-html ./cc)
4. Went through and checked to see if any of the individual lines (36) weren't covered. Two weren't.

Both of those lines are longer than 120 chars (162 and 141 respectively). That's the only issue with them. The change needed is to either concatenate or multi-line split the string that's being passed. There's no logic change there and they are both very easy to check for now that we know what and where they are.

Does that give you a bit more confidence that the change isn't as impact-ful as you believed?