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

Base PHP-cs-fixer migration to version 2 plus style fix #1555

Merged
merged 5 commits into from Dec 24, 2018

Conversation

Projects
None yet
3 participants
@massimilianobraglia
Copy link
Contributor

massimilianobraglia commented Dec 12, 2018

Hey there!

As stated here #1540, I'm opening multiple CS fix PRs.

This is the base porting from version 1 to version 2 of the linter. Also, I have applied the CS rules in order to show you how this should look like.

@ruflin and @p365labs: what's your opinion about this?

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 13, 2018

@massimilianobraglia This is great and it looks like it could still be reviewable as the changes overall are minimal.

"sebastian/phpcpd":"~2.0",
"squizlabs/php_codesniffer":"~2.5"
"phpdocumentor/phpdocumentor": "~2.8",
"friendsofphp/php-cs-fixer": "^2.0",

This comment has been minimized.

@ruflin

ruflin Dec 13, 2018

Owner

Could you have a look at CI? I assume some more updates are needed: https://travis-ci.org/ruflin/Elastica/jobs/467233662

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 13, 2018

Author Contributor

Ok! I'll check it ASAP

@ruflin
Copy link
Owner

ruflin left a comment

This change LGTM as soon as we get CI green. Let me know if I can help here.

Overall the changes are as minimal as I hoped for.

As soon as this PR is in, we should also create a follow up PR to runs the linting on Travis so we can block PR's which do not adhere.

Makefile Outdated
@@ -140,7 +140,7 @@ gource:

## DOCKER IMAGES

.PHONY: elastica-image
.PHONY: elastica-image

This comment has been minimized.

@ruflin

ruflin Dec 14, 2018

Owner

Is this change intentional?

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 14, 2018

Author Contributor

No, I didn't notice that 🤔
Fixing..

This comment has been minimized.

@ruflin

ruflin Dec 21, 2018

Owner

still needs a fix ;-)

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 21, 2018

Author Contributor

fixed 😄

} else {
return;
}
}

This comment has been minimized.

@ruflin

ruflin Dec 14, 2018

Owner

Nice that it catches such things with return. I'm positively surprised :-)

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 14, 2018

Author Contributor

Yeps! It's such a nice tool (and there are many rules that I have to look deeper 😄)

*/
public function testToArrayInvalidBucketsPath()
{
$this->expectException(\Elastica\Exception\InvalidException::class);

This comment has been minimized.

@ruflin

ruflin Dec 14, 2018

Owner

Are the also changes it has done automatically?

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 14, 2018

Author Contributor

Yes, it should be done by using the 'php_unit_dedicate_assert' => ['target' => 'newest'], rule

@@ -7,7 +8,7 @@
use Elasticsearch\Endpoints\Ingest\Pipeline\Put;
use Psr\Log\LoggerInterface;
class Base extends \PHPUnit_Framework_TestCase
class Base extends \PHPUnit\Framework\TestCase

This comment has been minimized.

@ruflin

ruflin Dec 14, 2018

Owner

Automated change?

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 14, 2018

Author Contributor

Yes. Obviously it should be imported with the use statement

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 14, 2018

Everything touched by this PR is automated by the CS-fixer. A part from the Makefile change :/ (probably is my mistake).

About the composer problem, it must be a version conflict for phpdocumentor/fileset (I think). Still I didn't have time to check it, what do you think about it @ruflin?

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from 4fb4606 to b478559 Dec 14, 2018

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 14, 2018

yes, seems like it. I wonder if setting it to 2.9 helps?

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from b478559 to 4541719 Dec 14, 2018

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 14, 2018

Unfortunately it does not help. I tried to remove it also and it does not work anyway.

Also, I noticed that the CS-Fixer does not support PHP 7.3 (see FriendsOfPHP/PHP-CS-Fixer#3697)...

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 17, 2018

The good news at least the errors change now. I'm opening #1567 so we can experimenting with updating the other dependencies separately.

For php-cs-fixer: When do you expect it to be compatible with PHP 7.3? Is it only a problem with 7.3 if it runs the linter or always? I'm thinking that on travis we should run linting as a separate step and not on all 4 versions.

@p365labs

This comment has been minimized.

Copy link
Collaborator

p365labs commented Dec 17, 2018

@massimilianobraglia I think we need to update composer.json here

env\elastica\composer.json

it uses "phpunit/phpunit":"~5.6"

not sure bad I have a bad smell on that version ;)

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 17, 2018

@ruflin the issue I linked is opened since April 2018. I do not know when the full compatibility will be merged. A comment states that the linter is compatible since we do not use one of the new features of PHP 7.3. I agree about not using the linter in every step but in another one.

I'm going to rebase and see if the build goes on 😄

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from 4541719 to 657da46 Dec 18, 2018

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 18, 2018

I made it! Let's see what the build on Travis says but the problem about composer's hangin' on was also with the phploc package (see 657da46#diff-bdfe059178421755f0677c4b7b332938L20)

I just removed it and it worked!

I have also updated PHPUnit version to ^6.0.

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 18, 2018

lmc-eu/steward#230

What about using this workaround until PHP-CS-fixer solves the issue with PHP 7.3?

@ruflin and @p365labs what do think about this?

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 20, 2018

I assume we don't need a workaround as we just run the linting job with 7.2?

Could you rebase this PR on top of master as @p365labs updated the dependency. I hope now the build will run.

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 20, 2018

I made it running. The build failed beacuse of PHP-CS-Fixer on PHP 7.3.

Rebasing will be done in a moment 😄

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from 657da46 to 4f15fb2 Dec 20, 2018

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 20, 2018

@ruflin and @p365labs
I removed php-cs-fixer from PHP 7.3 and added in .travis.yml the style check. Can you give it a look?

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from 7cb95ff to 84612d5 Dec 20, 2018

@p365labs

This comment has been minimized.

Copy link
Collaborator

p365labs commented Dec 20, 2018

@massimilianobraglia well done ! good point amending .travis + Make 👍

Show resolved Hide resolved env/elastica/Docker70 Outdated
@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 20, 2018

@@ -13,8 +13,6 @@
],
"require": {
"php": "^7.0",
"phpdocumentor/phpdocumentor": "^2.9",

This comment has been minimized.

@ruflin

ruflin Dec 21, 2018

Owner

Can we please keep this in? I need this to generate the docs :-)

This comment has been minimized.

@massimilianobraglia

massimilianobraglia Dec 21, 2018

Author Contributor

Ok :) Probably I forgot to readd that one.

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 21, 2018

My thinking for Travis was more that we introduce one more target which could be called Lint. And this target would have it's own Docker container, so no need to modify the other containers. We can still do this in a follow up PR to not further hold back this PR.

Could you readd the removed line to the composer file or does this still cause issue even with the updates?

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 21, 2018

It still causes the issue. No problem about adding a separate step with the linting :)
Let's see if I can manage it right now

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 21, 2018

the new travis.yml file will look something like this:

sudo: required
dist: trusty

group: edge


services:
  - docker

branches:
  only:
    - master

env:
  global:
    - DOCKER_VERSION=17.09.0
    - DOCKER_COMPOSE_VERSION=1.17.1
  matrix:
    - TARGET="70"
    - TARGET="71"
    - TARGET="72"
    - TARGET="73"
    - TARGET="Lint"

before_install:
  # check running "docker engine" and "docker-compose" version on travis
  - docker --version
  - docker-compose --version

  # list docker-engine versions, usefull to update the docker engine on travis and trusty
#  - sudo apt-cache madison docker-engine

  # update docker engine to the desired version
  - sudo apt-get -o Dpkg::Options::="--force-confnew" install -y docker-ce=${DOCKER_VERSION}~ce-0~ubuntu --allow-downgrades

  - sudo rm /usr/local/bin/docker-compose
  - curl -L https://github.com/docker/compose/releases/download/${DOCKER_COMPOSE_VERSION}/docker-compose-`uname -s`-`uname -m` > docker-compose
  - chmod +x docker-compose
  - sudo mv docker-compose /usr/local/bin

before_script:
  - sudo sysctl -w vm.max_map_count=262144

script:
  - if [ "$TARGET" != "Lint" ] ; then make tests TARGET=$TARGET ; fi
  - if [ "$TARGET" == "Lint" ] ; then make check-style ; fi

after_script:
  - cat /var/log/elasticsearch/*.log
  - cat /var/log/nginx/*.log
  - sudo rm composer.lock && sudo composer require satooshi/php-coveralls dev-master --no-ansi --no-progress --no-interaction

after_success:
  - bash <(curl -s https://codecov.io/bash)
  - vendor/bin/coveralls -v

What do you think about it, @ruflin?

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from eb2036b to 993540d Dec 21, 2018

@massimilianobraglia massimilianobraglia force-pushed the massimilianobraglia:migrate_cs_fixer_to_version_2 branch from 993540d to 41f890e Dec 21, 2018

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 21, 2018

@massimilianobraglia LGTM

@ruflin
Copy link
Owner

ruflin left a comment

This looks good, we are almost there. Last thing missing I think is a CHANGELOG entry.

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 21, 2018

What should I enter in the CHANGELOG?
Something like
Introduced new version of PHP-CS-Fixer and new Lint travis step?

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 21, 2018

@massimilianobraglia Changelog SGTM.

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 21, 2018

here you are 😄

@ruflin ruflin merged commit 419a508 into ruflin:master Dec 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 24, 2018

@massimilianobraglia Merged 🎄 Thank you so much for going through all these loops to get this in.

Moving forward we should make sure it's as easy as possible for contributors to apply the linting to not get stuck on this step in their PR's.

I'm happy to apply more linting rules to Elastica. The less we need to discuss about how things are formatted and the more we can focus on the code itself the better :-)

@massimilianobraglia

This comment has been minimized.

Copy link
Contributor Author

massimilianobraglia commented Dec 24, 2018

Yes!!
If you don't mind, I have the original rules ready and I can open another PR with that 😄

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Dec 27, 2018

@massimilianobraglia Yes, please. Best do 1 PR per change so we keep it small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment