8000 Fixed typos in the expectedException annotations by GrahamCampbell · Pull Request #19243 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Fixed typos in the expectedException annotations #19243

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fixed typos in the expectedException annotations #19243

wants to merge 3 commits into from

Conversation

GrahamCampbell
Copy link
Contributor
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

PHPUnit ignores any imports when resolving these. You must always reference the FQCN.

@GrahamCampbell GrahamCampbell mentioned this pull request Jun 30, 2016
Closed
@ro0NL
Copy link
Contributor
ro0NL commented Jun 30, 2016 •
edited
Loading

Not checked it fully, but i found -+ 34 violations. I.e. i guess there are more occurrences for this specific problem.

Also the root namespace is still missing now.. this annoys editors ;-)

@fabpot can you confirm these are false positives? Makes sense to test the namespaced exception is thrown... rather than some potential subclass.

fabpot
fabpot reviewed Jun 30, 2016
View reviewed changes
src/Symfony/Component/Asset/Tests/PackagesTest.php
@@ -57,7 +55,7 @@ public function testGetUrl()
}

/**
* @expectedException LogicException
* @expectedException Symfony\Component\Asset\Exception\LogicException
Copy link
Member

Choose a reason for hiding this comment

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

I think it should start with \ (unfortunately)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPUnit doesn't resolve them like that.

Copy link
Member

Choose a reason for hiding this comment

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

That's what we are using everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add them if you want, but phpunit will just trim them.

@GrahamCampbell
Copy link
Contributor Author

The FQCN is defined without a leading slash. The leading slash mearly indicates to escape to the global namespace. If your IDE doesn't understand that PHPUnit doesn't resolve thing in this way, then it's broken.

@ro0NL
Copy link
Contributor
ro0NL commented Jun 30, 2016 •
edited
Loading

Understand.. im fine with both. But lets be consistent :)

Still.. what if PHPunit fixes this, how should it be then? Cause potentially there can be a class Symfony\Component\Asset\Exception\LogicException relative to some namespace. And we have to add use statements (with maybe aliases) everywhere. I.e. adding a root namespace would be explicit and less error prone perhaps?

@GrahamCampbell
Copy link
Contributor Author

PHPUnit does do anything relative to a namespace. The fact that might be true is irrelevant.

@ro0NL
Copy link
Contributor
ro0NL commented Jun 30, 2016

Like i said my only concern is it requires a lot of refactoring whenever PHPunit implements this. Assuming its a valid usecase.. however i guess its not gonna happen because of this.

@GrahamCampbell
Copy link
Contributor Author

whenever PHPunit implements this

I don't think PHPUnit will ever implement this.

@keradus
Copy link
Member
keradus commented Jun 30, 2016

@ro0NL check this out: sebastianbergmann/phpunit#1633
for that, I really don't think PHPUnit will go that way, especially since it's somekinda dropping annotation in favor of ->expectException

@ro0NL
Copy link
Contributor
ro0NL commented Jun 30, 2016 •
edited
Loading

Sorry.. i cant really play with it right now, but to clearify.. this works different then as opposed to @throws, @return etc.

To sum up..

  • FQCN annotations from phpunit should be fully qualified (root namespace is optional, but pragmaticly prefered now for editors and is less error prone whener phpunit alters (IMHO))
  • FQCN annotations from phpdoc can be fully qualified (with root namespace) or partial (with use 8000 statement)

For both i would like to see a fixer optimizing this to some consistent variant.

edit: @keradus 👍

@fabpot
Copy link
Member
fabpot commented Jun 30, 2016

Thank you @GrahamCampbell.

fabpot added a commit that referenced this pull request Jun 30, 2016
@fabpot
minor #19243 Fixed typos in the expectedException annotations (Graham…
94773ad 
…Campbell)

This PR was squashed before being merged into the 2.7 branch (closes #19243).

Discussion
----------

Fixed typos in the expectedException annotations

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

PHPUnit ignores any imports when resolving these. You must always reference the FQCN.

Commits
-------

b36de36 Fixed typos in the expectedException annotations
@fabpot fabpot closed this Jun 30, 2016
@GrahamCampbell GrahamCampbell deleted the patch-1 branch June 30, 2016 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GrahamCampbell @ro0NL @keradus @fabpot @carsonbot
0