Skip to content

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

Merged
merged 19 commits into from
Jan 26, 2018
Merged

CI updates #53

merged 19 commits into from
Jan 26, 2018

Conversation

ashnazg
Copy link
Member

@ashnazg ashnazg commented Jan 13, 2018

No description provided.

@ashnazg ashnazg requested review from mvriel and jaapio January 13, 2018 15:21
Copy link
Member

@jaapio jaapio left a 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

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

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.

Copy link
Member Author

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;
Copy link
Member

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.

Copy link
Member Author

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,
Copy link
Member

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

Copy link
Member Author

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.
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

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

This description made sense.

Copy link
Member Author

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.

@ashnazg
Copy link
Member Author

ashnazg commented Jan 13, 2018

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

@jaapio
Copy link
Member

jaapio commented Jan 13, 2018

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
Copy link
Member Author

ashnazg commented Jan 13, 2018

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.

$actual = $context->getNamespaceAliases();

// sort so that order differences don't break it
$this->assertSame(sort($expected), sort($actual));
Copy link
Member Author

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.

@ashnazg
Copy link
Member Author

ashnazg commented Jan 16, 2018

I think this is ready now.

@ashnazg ashnazg force-pushed the ci branch 2 times, most recently from f24036c to d9b62e1 Compare January 16, 2018 13:23
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
Copy link
Member

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?

Copy link
Member Author

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.

@jaapio jaapio merged commit 54760a1 into phpDocumentor:master Jan 26, 2018
@ashnazg 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
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.

2 participants