Skip to content

Remove expected exception annotations#1993

Merged
teohhanhui merged 1 commit into
api-platform:2.2from
bendavies:remove-expected-exception-annotation
Jun 5, 2018
Merged

Remove expected exception annotations#1993
teohhanhui merged 1 commit into
api-platform:2.2from
bendavies:remove-expected-exception-annotation

Conversation

@bendavies

Copy link
Copy Markdown
Contributor
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

#1988 (comment)

@bendavies bendavies changed the title Remove expected exception annotation Remove expected exception annotations Jun 1, 2018
Comment thread .php_cs.dist Outdated
->in(__DIR__)
->exclude('tests/Fixtures/app/cache')
->in([__DIR__.'/src', __DIR__.'/tests'])
->append([__FILE__])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well, to apply the same standard to app php files in the project.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, what's wrong with just using

->in(__DIR__)

as before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, reverted!

Comment thread .php_cs.dist
* file that was distributed with this source code.
*/

declare(strict_types=1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(But if we really want to do this, then let's do it right...)

Missing blank line after.

Comment thread .php_cs.dist Outdated
return PhpCsFixer\Config::create()
->setRiskyAllowed(true)
->setRules([
'@PHPUnit60Migration:risky' => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you take this opportunity to restore the alphabetical order? 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well, it's not at the moment....done anyway

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I saw that the order was out of whack. You could put it before the @Symfony rules, but never mind...

@bendavies

bendavies commented Jun 1, 2018 via email

Copy link
Copy Markdown
Contributor Author

*/
public function testResolveOperationPathWithSubresource()
{
$this->expectException(\ApiPlatform\Core\Exception\InvalidArgumentException::class);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be imported instead of using the FQN. It's weird that PHP CS Fixer doesn't fix that automatically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Erg yeah....this is going to be annoying.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dunglas done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because PHP-CS-Fixer does not have such a fixer. 😆

@teohhanhui teohhanhui Jun 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The closest thing is: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v2.12.0/src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

but it only deals with existing imports. (Oh, and only for type hints...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

already fixed :)

@bendavies

Copy link
Copy Markdown
Contributor Author

@teohhanhui done

Comment thread .php_cs.dist Outdated
@@ -1,5 +1,15 @@
<?php

/*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of having this header block in a file like .php_cs.dist, but oh well...

Comment thread .php_cs.dist Outdated
$finder = PhpCsFixer\Finder::create()
->in(__DIR__)
->exclude('tests/Fixtures/app/cache')
->in([__DIR__.'/src', __DIR__.'/tests'])

@teohhanhui teohhanhui Jun 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bendavies I thought you've reverted this. We do want to include files in bin and public too, for example.

Can you explain the rationale for this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i did mean to, sorry. fixing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reverted.

Comment thread .php_cs.dist Outdated
->exclude('tests/Fixtures/app/cache')
->in([__DIR__.'/src', __DIR__.'/tests'])
->append([__FILE__])
->exclude(__DIR__.'/tests/Fixtures/app/cache')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you revert all the changes from this line and above? But we can keep the

declare(strict_types=1);

of course.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

which bits exactly. it is ok now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll take care of the rest. 😄

@teohhanhui teohhanhui mentioned this pull request Jun 5, 2018
@teohhanhui teohhanhui merged commit 99f2724 into api-platform:2.2 Jun 5, 2018
@teohhanhui

Copy link
Copy Markdown
Contributor

Thanks @bendavies! 🎉

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.

3 participants