CI updates #53

ashnazg · 2018-01-13T15:21:03Z

No description provided.

… names of Foo_;

jaapio

Some important changes should be reverted

jaapio · 2018-01-13T19:59:34Z

easy-coding-standard.neon

@@ -19,3 +19,5 @@ parameters:
    skip:
        PHP_CodeSniffer\Standards\Generic\Sniffs\NamingConventions\CamelCapsFunctionNameSniff:
            - */tests/**
+        PHP_CodeSniffer\Standards\Squiz\Sniffs\Classes\ValidClassNameSniff:
+            - */src/**


Think it is better to exclude specific file instead of excluding all.

My original thought was that because there were so many (13 of the >20 classes) that when I had time to dig into it, I might discover that the classes involved were all internal classes, and thus I could refactor their names without a BC break. That's why I went with a blanket src here.

jaapio · 2018-01-13T20:00:27Z

tests/unit/Types/ContextFactoryTest.php

@@ -192,5 +198,5 @@ public function tearDown()

 namespace phpDocumentor\Reflection\Types\Mock {
    // the following import should not show in the tests above
-    use phpDocumentor\Reflection\DocBlock\Description;


This shouldn't be removed.

Maybe if I add a dummy class in that namespace that utilizes that import, then ECS won't complain.

jaapio · 2018-01-13T20:01:38Z

tests/unit/Types/ContextFactoryTest.php

    use Mockery as m;
-    use phpDocumentor\Reflection\DocBlock,


These imports are part of the unittest. please add them back

They're still in there... it looks like ECS just reorders them.

jaapio · 2018-01-13T20:02:51Z

src/Types/Object_.php

@@ -55,15 +54,10 @@ public function getFqsen()
        return $this->fqsen;
    }

-    /**
-     * Returns a rendered output of the Type as it would be used in a DocBlock.


Think the docblock here makes sense. The @return doesn't

What I actually did here was to move the docblock to the interface method, rather than it being located in this implementor.

jaapio · 2018-01-13T20:03:49Z

src/TypeResolver.php

@@ -130,26 +124,25 @@ public function resolve($type, Context $context = null)
    /**
     * Analyse each tokens and creates types
     *
-     * @param \ArrayIterator $tokens  the iterator on tokens
-     * @param Context        $context
-     * @param integer        $parserContext on of self::PARSER_* constants, indicating


This description made sense.

It looks like ECS chose to remove the @param Context $context tag, based on:

no description in the tag

typehint already in the method signature

It doesn't look like it altered any existing descriptions, other than leading spaces on the second line of $parserContext's description.

ashnazg · 2018-01-13T21:24:46Z

I think nearly all of the changes were the "--fix" results of ECS... (afk)

jaapio · 2018-01-13T21:33:38Z

I understand that but ECS doesn't understand what is tested. Think however if the tests were written correctly they would have been failing at this point.

ashnazg · 2018-01-13T22:53:59Z

The continued passing of the tests was one reason I thought the ecs --fix changes must be ok. That, and the assumption that it was changes like this that the other ecs --fix to green the build changesets were doing. Were the other ecs --fix changes actually a combination of "some fixes" and "some explicit ignores"? If so, then those would come down to judgment calls, and I'd best not be trying to make those decisions myself.

ashnazg · 2018-01-13T23:18:21Z

tests/unit/Types/ContextFactoryTest.php

+            $actual = $context->getNamespaceAliases();
+
+            // sort so that order differences don't break it
+            $this->assertSame(sort($expected), sort($actual));


The reordering of the use imports explains why I had to add the sort() behavior around the two arrays here... I thought that easier than trying to assume the ordering of the array members at runtime. It now looks like the runtime ordering actually was consistent and based on the import ordering.

use travis_retry for occasional failure points;

…tions with reflection-* deps

ashnazg · 2018-01-16T03:32:45Z

I think this is ready now.

jaapio · 2018-01-17T20:00:15Z

appveyor.yml

@@ -45,10 +45,12 @@ install:
  - IF NOT EXIST php-installed.txt echo assert.exception=On >> php.ini
  - IF NOT EXIST php-installed.txt appveyor DownloadFile https://getcomposer.org/composer.phar
  - IF NOT EXIST php-installed.txt echo @php %%~dp0composer.phar %%* > composer.bat
+  - IF NOT EXIST php-installed.txt appveyor DownloadFile https://phar.phpunit.de/phpunit.phar
+  - IF NOT EXIST php-installed.txt echo @php %%~dp0phpunit.phar %%* > phpunit.bat


is there a specific reason to not use phive on appveyor?

That change is gone now... went back to require-dev having phpunit.

CI updates Former-commit-id: 54760a1

ashnazg added 10 commits January 12, 2018 16:23

move method docblock to interface

1462915

move some logic into private methods in order to do closer type checking

ec92571

ignore esc

d65e7b5

auto fixes from ecs

8967807

ignore camel case classname sniff for now, to avoid refactoring class…

c7db5f5

… names of Foo_;

fix tags

f2e13c5

sort arrays so that order differences don't break the tests

0cdd493

fix tags

08e8b8f

need to handle null in addition to '';

9d9b088

fix tags

27a8a14

ashnazg requested review from mvriel and jaapio January 13, 2018 15:21

jaapio requested changes Jan 13, 2018

View reviewed changes

ashnazg commented Jan 13, 2018

View reviewed changes

ashnazg added 6 commits January 13, 2018 17:28

explicitly list classes that need to be ignored

d4252e2

add a dummy class so that the imported namespace is utilized

09ed872

remove duplicated checker

23f3a7d

another ecs fix

2053665

use phive and phars for tools;

9b55160

use travis_retry for occasional failure points;

I think phpunit's reflection-docblock is interfering with our tests

abe3096

ashnazg force-pushed the ci branch from d0f0a84 to abe3096 Compare January 15, 2018 17:33

ashnazg added 2 commits January 15, 2018 13:17

do not use phpunit.phar against a package whose deps are in that phar

f1abee4

stick with phpunit in require-dev, for IDE use and to lessen complica…

714e94c

…tions with reflection-* deps

use --dev phpunit

d9b62e1

ashnazg force-pushed the ci branch 2 times, most recently from f24036c to d9b62e1 Compare January 16, 2018 13:23

jaapio reviewed Jan 17, 2018

View reviewed changes

jaapio approved these changes Jan 26, 2018

View reviewed changes

jaapio merged commit 54760a1 into phpDocumentor:master Jan 26, 2018

ashnazg deleted the ci branch January 26, 2018 19:36

jeremiemychezmoi pushed a commit to jeremiemychezmoi/TypeResolver that referenced this pull request Jul 9, 2019

Merge pull request phpDocumentor#53 from ashnazg/ci

797220d

CI updates Former-commit-id: 54760a1

Uh oh!

CI updates #53

CI updates #53

Uh oh!

Conversation

ashnazg commented Jan 13, 2018

Uh oh!

jaapio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashnazg commented Jan 13, 2018

Uh oh!

jaapio commented Jan 13, 2018

Uh oh!

ashnazg commented Jan 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashnazg commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!