-
-
Notifications
You must be signed in to change notification settings - Fork 50
CI updates #53
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
CI updates #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some important changes should be reverted
easy-coding-standard.neon
Outdated
@@ -19,3 +19,5 @@ parameters: | |||
skip: | |||
PHP_CodeSniffer\Standards\Generic\Sniffs\NamingConventions\CamelCapsFunctionNameSniff: | |||
- */tests/** | |||
PHP_CodeSniffer\Standards\Squiz\Sniffs\Classes\ValidClassNameSniff: | |||
- */src/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it is better to exclude specific file instead of excluding all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if I add a dummy class in that namespace that utilizes that import, then ECS won't complain.
use Mockery as m; | ||
use phpDocumentor\Reflection\DocBlock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These imports are part of the unittest. please add them back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're still in there... it looks like ECS just reorders them.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think the docblock here makes sense. The @return
doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I actually did here was to move the docblock to the interface method, rather than it being located in this implementor.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description made sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I think nearly all of the changes were the "--fix" results of ECS... (afk) |
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. |
The continued passing of the tests was one reason I thought the |
$actual = $context->getNamespaceAliases(); | ||
|
||
// sort so that order differences don't break it | ||
$this->assertSame(sort($expected), sort($actual)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
I think this is ready now. |
f24036c
to
d9b62e1
Compare
appveyor.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason to not use phive on appveyor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is gone now... went back to require-dev having phpunit.
CI updates Former-commit-id: 54760a1
No description provided.