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

php-cs-fixer introduced #173

Merged
merged 10 commits into from Nov 1, 2016

Conversation

nstapelbroek
Copy link
Contributor

Hi!

I've added a codestyle checker and fixed some codestyle issues in the src code.
Please let me know you you'll need anything changed.

Some notes:

  • I did not touch the test directory, but I'm happy to do so if requested.
  • Since fixing the method name violations in src/VCR/Util/StreamProcessor.php and src/VCR/LibraryHooks/StreamWrapperHook.php would break the API, I've added an exclude to the rules instead. I would be happy to change this, but I'm not sure if its possible or if you simply don't want to because of API.

throw new \LogicException(
"The request does not match a previously recorded request and the 'mode' is set to '{$this->config->getMode()}'. "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this {$this->config->getMode()} works. I've removed it when adding sprintf 🙈

@renatomefi
Copy link
Member

@nstapelbroek thanks for your contribution!

Can you put a phpcs file configuration? Also make the test available at composer test, this way you don't have to pass the flags while calling it on travis.

Also when you put the config file let's see which kind of rules do we want and how they would look like in the project!

@renatomefi
Copy link
Member

Oh, and I'm comfortable having only the php-cs-fixer in the repository. So no need for phpcs. You can add the --dry-run and --diff arguments to the command when running the fixer. If it returns some changes the result code would be 1, failing the build. 👍

@nstapelbroek
Copy link
Contributor Author

Updated the code, let me know if you need any additional adjustments 😄

Copy link
Member

@renatomefi renatomefi left a comment

Choose a reason for hiding this comment

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

Hey Nico, it looks good, just 2 comments!

$cacheDir = getenv('TRAVIS') ? getenv('HOME') . '/.php-cs-fixer' : __DIR__;
$config->setDir($cacheDir);

return $config;
Copy link
Member

Choose a reason for hiding this comment

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

Missing new line!

@@ -62,7 +62,7 @@ public function testIterateFirstNestedObject()
public function testIterateSecondNestedObject()
{
$this->iterateAndTest(
"-". "\n"
"-" . "\n"
Copy link
Member

Choose a reason for hiding this comment

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

can you put a rule for double and single quotes?

Copy link
Member

Choose a reason for hiding this comment

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

use the single_quote rule

Copy link
Member

Choose a reason for hiding this comment

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

canso you also check if there is any change if you enable braces? I feel like some old if statements without braces are not being fixed, might be an impression tho!

@nstapelbroek
Copy link
Contributor Author

I've added the requested rules. Personally I think the single quote rule on the php-cs-fixer creates more inconsistency then it prevents. My preference would be to refactor this in a separate issue.

If you agree on the changes, you can merge anyway 😄

@renatomefi
Copy link
Member

@nstapelbroek why do you think it generates more inconsistency?

@renatomefi renatomefi changed the title codestyle checks php-cs-fixer introduced Oct 31, 2016
@nstapelbroek
Copy link
Contributor Author

@renatomefi i think the suggestion was good. Only the php-cs-fixer made things more inconsistent because it wont touch strings with a variable. Honestly, we should also take care of the double quoted lines before merging this. What do you think?

@renatomefi
Copy link
Member

@nstapelbroek but it's ok if the double quotes actually have variables, what do you think?

@renatomefi renatomefi merged commit 7c76cd2 into php-vcr:master Nov 1, 2016
@renatomefi
Copy link
Member

As we discussed I think it's ok to have double quotes to the special strings! If you want you can do the same for https://github.com/php-vcr/phpunit-testlistener-vcr

Thank you very much!

@nstapelbroek
Copy link
Contributor Author

Thank you for taking the time to review!

Awesome. Will probably work on the testlistener-vcr this weekend.

On Tue, Nov 1, 2016 at 8:31 PM Renato Mendes Figueiredo <
notifications@github.com> wrote:

As we discussed I think it's ok to have double quotes to the special
strings! If you want you can do the same for
https://github.com/php-vcr/phpunit-testlistener-vcr

Thank you very much!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#173 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADNkUo5RGjgx0NVMeM0KPQdAWCAKcgZ1ks5q55OsgaJpZM4KkNXT
.

tstuttard added a commit to tstuttard/php-vcr that referenced this pull request Aug 25, 2017
* master:
  Fix logo image
  Travis: skip phpcs in case of integration tests
  stream_lock operation can use strict type checking when it's 0
  PHP 7.1: Fixed issue with php-cs-fixer
  Fix bug with StreamProcessor::stream_lock explained here antecedent/patchwork#27
  Test php 7.1
  test for setting post method when CURLOPT_POSTFIELDS set
  Adds github pull request template
  Set POST method when CURLOPT_POSTFIELDS is set
  Fix parallel cURL requests:  - Make sure all handles are inside an array so they can be removed properly when done  - Remove handle as soon as done  - Fix behaviour of curl_multi_exec to return last curl info  - Also fix test, check for the second to last curl info and make sure the third returns false, not the second  - CS fixes
  php-cs-fixer introduced (php-vcr#173)
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.

None yet

2 participants