Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable tests in CodingStyle directory #4340

Merged
merged 5 commits into from
Sep 5, 2016

Conversation

mlocati
Copy link
Contributor

@mlocati mlocati commented Sep 5, 2016

Let's include in the test suite the tests introduced in #4157

@a3020
Copy link
Contributor

a3020 commented Sep 5, 2016

Tabs alert :)

@mlocati
Copy link
Contributor Author

mlocati commented Sep 5, 2016

Tabs alert :)

Well, even if it's a good start, we could not only test for tabs: there are a lot of style checks that could be done.
Maybe @KorvinSzanto has already done some tests about coding style checks?

<directory>tests/tests/Attribute/</directory>
<directory>tests/tests/AttributeValue/</directory>
<directory>tests/tests/Block/</directory>
<directory>tests/tests/CodingStyle/</directory>
Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to this, @mlocati

@aembler
Copy link
Member

aembler commented Sep 5, 2016

I would really rather not have tests fail if coding standards aren't met. But the short tag check is good.

@aembler aembler merged commit 336b6ba into concretecms:develop Sep 5, 2016
@mlocati mlocati deleted the enable-CodingStyle-tests branch September 5, 2016 14:06
@KorvinSzanto
Copy link
Member

I swear to god I replied to this thread haha. I think we should do this with csfixer or phpmd. We can just add that as a script in .travis like i did with this project here: https://github.com/buttress/browserslist/blob/master/.travis.yml#L32

I definitely don't think it's worth the cpu to do this ourselves in phpunit so I'd like to update this to use something that is purpose made.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 27, 2016

Well, style handling in view files (mix of php and html) is a nightmare.
I guess that readable view files won't pass coding style checks, and view files that pass coding style checks aren't readable that much (see for instance the mess introduced in view files by php-cs-fixer).
So, we have 2 options:

  1. include all the files in the checks -> they won't pass or we'd need messy view files
  2. exclude view files from tests -> short tags may and un in view files

@mlocati
Copy link
Contributor Author

mlocati commented Sep 27, 2016

I swear to god I replied to this thread haha

You replied here: #4380

@KorvinSzanto
Copy link
Member

@mlocati this is what I have in my scrutinizer branch right now. It only runs on one machine:

$ concrete/vendor/bin/phpcs -p --standard=psr2 --sniffs=Generic.PHP.DisallowShortOpenTagSniff,Generic.WhiteSpace.DisallowTabIndent concrete/src concrete/controllers

This works great with #4419 merged

@mlocati
Copy link
Contributor Author

mlocati commented Sep 27, 2016

My manual and stupid (but effective) check shows a red flag on tests for every php file.

Is this possible with your approach (not limited to controllers or src directories)?

@KorvinSzanto
Copy link
Member

Yeah, just use

$ concrete/vendor/bin/phpcs -p --standard=psr2 --sniffs=Generic.PHP.DisallowShortOpenTagSniff,Generic.WhiteSpace.DisallowTabIndent concrete

But there are quite a lot of tab vs space issues in view files and I figured it's not really worth it anyway. IMO better get the classes solid than have overly permissive code style checks.

KorvinSzanto pushed a commit that referenced this pull request Dec 12, 2016
* Enable tests in CodingStyle directory

* Enable PHP short open tags in TravisCI

* Fix phpunit.xml indentation

* Use HHVM-specific ini file

* Check hhvm.enable_short_tags instead of short_open_tag on HHVM


Former-commit-id: 336b6ba
Former-commit-id: 50ec903bcd2064febd17fd0383cfdef37aad9ee7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants