Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Enhancement: Run PHP_CodeSniffer on Travis #6182

Closed
wants to merge 13 commits into from

Conversation

localheinz
Copy link
Member

This PR

  • updates .travis.yml to run phpcs on Travis
  • adds an XML configuration for phpcs
  • removes the dependency on fabpot/php-cs-fixer
  • updates README-GIT.md in regard to suggested GIT pre-commit hook, use phpcs instead of php-cs-fixer

Let's see how much pain this is going to cause.

/cc @Maks3w

@localheinz localheinz changed the title [WIP] Enhancement: Run PHP_CodeSniffer on Travis Enhancement: Run PHP_CodeSniffer on Travis Apr 23, 2014
@danizord
Copy link
Contributor

👍

@localheinz
Copy link
Member Author

Seems like this much pain: :hurtrealbad:

@@ -41,7 +41,8 @@
"fabpot/php-cs-fixer": "dev-master#2f1df1d",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this? It's a damn pain every second upgrade :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do after I got up!

@Maks3w
Copy link
Member

Maks3w commented Apr 24, 2014

Two things:

  • Only display errors
  • Could we have a config file like .php_cs?

@localheinz
Copy link
Member Author

@Maks3w

Doesn't look like the script accepts a config file:

https://github.com/squizlabs/PHP_CodeSniffer/blob/1.5.2/CodeSniffer.php#L2162-L2165

@Maks3w
Copy link
Member

Maks3w commented Apr 24, 2014

May will need to write an XML with the rules for the project.

@Maks3w Maks3w added this to the 2.4.0 milestone Apr 24, 2014
@Maks3w
Copy link
Member

Maks3w commented Apr 24, 2014

By the moment I've tagged this for next major release just because may add a lot of git conflicts with the PRs in the queue

@localheinz
Copy link
Member Author

@Maks3w

Rebased and updated to use an XML configuration file.

Still failing, of course.

@DASPRiD
Copy link
Member

DASPRiD commented Jul 12, 2014

Shouldn't we in the same run also remove the php-cs-fixer abuse as a checker?

@Ocramius
Copy link
Member

@DASPRiD yeap

@localheinz
Copy link
Member Author

@DASPRiD @Ocramius

I've also removed the dependency on fabpot/php-cs-fixer and removed the note about optionally adding a Git pre-commit hook.

What do you think?

From what I believe was agreed upon in PHP-FIG ages ago, it would be great to see component maintainers make an effort to get contributors to fix all of these issues. I'm willing to put in some work, too. How about a coordinated effort?

@localheinz
Copy link
Member Author

@Maks3w

Can you provide a list of rules that should be added to phpcs.xml in order to reflect the rules ZF commits to?

This was referenced Jul 13, 2014
@Ocramius
Copy link
Member

That is... quite a report :| but I like it a lot!

@Ocramius
Copy link
Member

Wondering how much it'd take to fix all those or if we should delay it for when 2.4 is released...

@chadicus
Copy link
Contributor

@Ocramius I opened a PR #6454, but soon realized that PR would be over whelming to review.
Taking @localheinz suggestion I'm separating it per component. Hopefully I'll help take a big chunk out of those 6410 errors.
The first of these is #6490

@Ocramius
Copy link
Member

@chadicus I'll start merging those soon.

@Ocramius
Copy link
Member

Ocramius commented Aug 6, 2014

Checked this out locally and rebased it:

--------------------------------------------------------------------------------
A TOTAL OF 6048 ERROR(S) AND 0 WARNING(S) WERE FOUND IN 1042 FILE(S)
--------------------------------------------------------------------------------

So yeah: progress \o/

Ocramius added a commit to Ocramius/zf2 that referenced this pull request Aug 6, 2014
@Ocramius
Copy link
Member

Ocramius commented Aug 6, 2014

Also see localheinz#2, which includes parallelization of the CS fixes

Ocramius added a commit to Ocramius/zf2 that referenced this pull request Aug 6, 2014
Ocramius added a commit to Ocramius/zf2 that referenced this pull request Aug 6, 2014
localheinz added a commit to localheinz/zf2 that referenced this pull request Sep 16, 2014
…cks-parallelization

zendframework#6182 - running PHPCS checks in parallel
@Ocramius
Copy link
Member

Fair enough. Thoughts, @localheinz?

@keradus
Copy link
Contributor

keradus commented Dec 16, 2014

BTW, when releasing new version I made some regression on Symfony full stack framework and... ZF2 ;)
That's why you see my recent PRs here ;)

@localheinz
Copy link
Member Author

@Ocramius

To me it's not about the tool, but about the results, no? Whichever tool helps to

  • fix coding style issues
  • enforce the coding style committed to

is a tool that gets the job done.

@localheinz
Copy link
Member Author

Not really related to silexphp/Silex#1079.

@keradus
Copy link
Contributor

keradus commented Dec 16, 2014

Both can do that, and PHP CS Fixer is more mature than PHPCBF.
And looking at community PHP CS Fixer have slightly bigger one.

@localheinz
Copy link
Member Author

--------------------------------------------------------------------------------
A TOTAL OF 4338 ERRORS AND 0 WARNINGS WERE FOUND IN 553 FILES
--------------------------------------------------------------------------------
PHPCBF CAN FIX 3661 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Yay!

@keradus
Copy link
Contributor

keradus commented Dec 16, 2014

No need to post same thing in both places:
silexphp/Silex#1079 (comment)
(see last item in list)

@Ocramius
Copy link
Member

Taking an opinionated decision and closing as won't fix here.

The reasons behind this change of direction are following:

  • @keradus has been quite collaborative, and as a core-contributor to the fixer, he knows his way around php-cs-fixer. I hope he'll stick around :-)
  • php-cs-fixer is now stable, and I trust that the stability will be maintained over time
  • php-cs-fixer is now a community project, and not just under @fabpot's wing (he obviously currently has no time to look at it closely)
  • php-cs-fixer seems to have fixed all the horrible regex-parsing issues by moving to lexer-based parsing of code to be fixed (see Text within strings is changed PHP-CS-Fixer/PHP-CS-Fixer#152) /cc @DASPRiD
  • we are using single fixer rules and improving CS checks one at a time, which makes a lot of sense in the diffs, and reduces likeliness of merge conflicts

@localheinz thanks for bringing up the issue though, as we really got very far by just working on this topic! It was really useful to flesh things out :-)

That said, php-cs-fixer will ONLY stay as a build dependency, used ONLY to CHECK if things are wrong: running the fixer on the codebase may still cause unexpected issues, and I want to be very cautious on those.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants