Resolves #170.

[guillermoandrae]

No description provided.

[GrahamCampbell]

We could just use https://styleci.io/ you know. :)

[guillermoandrae]

@GrahamCampbell there are a few options, really, but I'm just going by what was requested here: #170

[GrahamCampbell]

StyleCI would fail on pulls like travis does, only separately, so it makes total sense. NB, StyleCI didn't exist when that other discussion took place. It's meant to be a game changer, but I'm biased, lol.

[guillermoandrae]

I'm impartial. We could actually use Scrutinizer for tests as well. Or any of the many other CI services in existence. Doesn't matter to me.

[GrahamCampbell]

The trouble with scrutinizer is it won't provide you with a diff you can just apply with git in 1 second.

[guillermoandrae]

@stof and @pilot and others can weigh in. StyleCI looks promising. I don't have a horse in this race, though, so I'm willing to go with whatever they decide works best for the project at the moment. I'll give StyleCI a shot for a personal project or two, though.

[xabbuh]

Missing final newline.

[guillermoandrae]

@xabbuh done.

[stof]

your scrutinizer config does not even make things fail when there are new CS issues (you don't define any failure condition).

If your goal is to report only CS issues, styleci is probably a better choice. The main advantage of Scrutinizer is that it provides static analysis of the code.

On a side note, I'm not a fan of making function signatures multiline. And PSR-2 does not enforce a max length

[GrahamCampbell]

If your goal is to report only CS issues, styleci is probably a better choice. The main advantage of Scrutinizer is that it provides static analysis of the code.

Yeh, I don't use scrutinizer for style checks - it's not what it's best at. It is super useful for finding my typos and highlighting other errors. I think it's really nice to have TravisCI to run tests, StyleCI, to check cs, and Scrutinizer to fix all issues where tests don't cover things well enough.

[GrahamCampbell]

On a side note, I'm not a fan of making function signatures multiline. And PSR-2 does not enforce a max length

I agree. StyleCI won't enforce this, since it strictly does do what PSR-2 says in that context, so does not enforce a line limit, or rather, php-cs-fixer behaves like that, lol.

[guillermoandrae]

I'm not comfortable enough with StyleCI to add the configuration file. I dug around and found the latest post about configuration, but... I don't know... does this really seem mature enough for us to use on this project, considering there is "essentially no documentation at the moment"? @GrahamCampbell I get that you want to promote your app, but it just seems like you're still working out some of the kinks.

I can modify the scrutinizer config to make it fail the build if changes aren't PSR2-compliant. Or I can take out the scrutinizer config altogether and we can just merge this PR (after I modify the multiline function definition) and you guys can work out how to enforce the style check in another PR/issue.

[guillermoandrae]

@stof: regarding that multiline function declaration... phpcs generates a warning when I revert the change:

----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 147 | WARNING | Line exceeds 120 characters; contains 124 characters
----------------------------------------------------------------------

Should I modify the severity in the style check?

P.S. A better question, IMO, is: why does that function need to take so many arguments? Generally speaking, I'm a big fan of Clean Code's 3 or less recommendation.

[stof]

@guillermoandrae PSR-2 does not forbid long lines (it only recommends to avoid them, but does not make it mandatory).
And I agree with you that this method should probably be refactored to provide a simpler signature (but keeping the support for the current one to preserve BC)

[guillermoandrae]

@stof re: PSR-2 -- I know, but phpcs warns you by default if the line exceeds 120 characters. So leaving it at 124 would cause the build to fail if we were checking with the default severity.

[GrahamCampbell]

does this really seem mature enough for us to use on this project, considering there is "essentially no documentation at the moment"? @GrahamCampbell I get that you want to promote your app, but it just seems like you're still working out some of the kinks.

Actually, we're very mature. We're using php-cs-fixer that's been in development for used, and is uset by fabbot.io on symfony/symfony.

[GrahamCampbell]

PSR-2 -- I know, but phpcs warns you by default if the line exceeds 120 characters. So leaving it at 124 would cause the build to fail if we were checking with the default severity.

PHPCS is doing it wrong then. PSR-2 says lines of any length are fine.

[guillermoandrae]

Options:

1. Merge this pull request without the use of a style check service and create another issue/PR so that someone more familiar with StyleCI can add the configuration file.
2. Have Scrutinizer fail the build if the PR doesn't pass checks.
3. Use php-cs-fixer.
4. Close this PR.

Those are the options as I see them. Please advise.

[GrahamCampbell]

Regarding option 1, styleci's config is basically the same as php-cs-fixer's. We would have used php-cs-fixer's config on styleci if it weren't for the fact it was actually a php file... we can't require user provided code on our workers...

[pilot]

@guillermoandrae rebase pls and check the test fails

[cursedcoder]

can I take this @guillermoandrae ?

[guillermoandrae]

@cursedcoder go for it

[GrahamCampbell]

This can be closed now as StyleCI is already enabled on this repo and php-cs-fixer config is present.