-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: introduce PhpUnitAttributesFixer
#7831
feat: introduce PhpUnitAttributesFixer
#7831
Conversation
8c12a44
to
218f687
Compare
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.
Quick feedback after taking a look 😉. More when it comes out of the draft.
5b173ad
to
537eb1b
Compare
private static function fixWithSingleStringValue(Tokens $tokens, int $index, Annotation $annotation): array | ||
{ | ||
Preg::match( | ||
sprintf('/@%s\s+(.*\S)(?:\R|\s*\*+\\/$)/', $annotation->getTag()->getName()), |
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 regex would be good to be deduplicated
- link to a parsing phpunit source would be good so the parsing can be checked & documented if 1:1 with native phpunit
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 regex would be good to be deduplicated
done
- link to a parsing phpunit source would be good so the parsing can be checked if 1:1 with native phpunit
I'd rather not, for 2 reasons:
- it is not a single place:
- https://github.com/sebastianbergmann/phpunit/blob/11.0.3/src/Metadata/Parser/AnnotationParser.php#L63
- https://github.com/sebastianbergmann/phpunit/blob/11.0.3/src/Metadata/Parser/Annotation/DocBlock.php#L41-L45
also, it was made differently in older PHPUnit: https://github.com/sebastianbergmann/phpunit/blob/9.6.16/src/Util/Annotation/DocBlock.php
- let's focus on providing good tests, so they keep the fixer working properly
Co-authored-by: Michael Voříšek <mvorisek@mvorisek.cz>
2d5b6e9
to
5991162
Compare
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.
Awesome work @kubawerlos 👏. Personally I would drop handle
from test cases' names because when you consider --testdox
output it looks weird (e.g. "Fix with handle·After"). Actually I would switch to testdox entirely, but it's a separate story 😉. Anyway, only one minor comment from me, please merge on your own with or without the fix.
it would be great to have an option to add the attributes without removing the annotation (which also requires checking if the attribute is already there to avoid "fixing" it multiple times). When you need to support both PHPUnit 11 (to support the latest PHPUnit version) and PHPUnit 9.6 (to support older PHP versions), the way to go is to define both the attributes (for PHPUnit 10+) and the annotation (used by PHPUnit 9 and ignored in 10+ when the attributes are present). |
#7832 and #7833 are reverted here - it was a (naive) thinking, a better approach is NOT to migrate to attributes that are not supported in a given scope (like
@large
and@medum
are fine on method level, but#[Large]
and#[Medium]
are not allowed ion method level).