Skip to content
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: Add support for attribute arguments in AppliesAttribute #319

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

raffaelecarelle
Copy link
Contributor

Idea

Modify ´AppliesAttribute´ selector for search a class that applies attribute with certain argument value (if passed, otherwise the behavious is like previously PR).

Limitations

Arguments check works if attributes is defined with ´Named Parameters´

@carlosas
Copy link
Owner

Also update the docs please 🙂 docs/documentation/selectors.md

@raffaelecarelle raffaelecarelle force-pushed the AppliesAttributeWithArgumentsRule branch 2 times, most recently from 84493a8 to dc52826 Compare February 14, 2025 11:24
@raffaelecarelle
Copy link
Contributor Author

hello @carlosas thank for the feedback. I've done the fixes. Let me know if now is ok

@carlosas
Copy link
Owner

I did a checkout in my local and run the tests manually, and I found they are failing. There is now a new phpunit folder that is not being run by the workflow. I updated the CI configuration to include it. Could you merge/rebase the current master branch and fix the issues? 🙏

Captura de pantalla 2025-02-14 a las 13 32 13

@carlosas
Copy link
Owner

Also something is not ok in the coding standards on PHP 7.4. I increased the verbosity to see which rule is failing in the next run

Modified AppliesAttribute to handle optional attribute arguments for precise matching. Updated relevant methods, fixtures, and added unit tests to ensure functionality with arguments and regex-based matching.
Tests and logic are updated to bypass attribute matching when running on PHP versions earlier than 8.0. This ensures compatibility and prevents failures related to unsupported features.
Reordered constructor parameters and renamed `attributeName` to `classname` for improved clarity and consistency. Updated all affected method calls and tests to reflect these changes. Removed redundant comments and unused code for better maintainability.
Added clarification on passing multiple arguments with values to the `Selector::appliesAttribute` method. This improves the readability and understanding of the documentation for users.
@raffaelecarelle raffaelecarelle force-pushed the AppliesAttributeWithArgumentsRule branch from c4d8a18 to cc0c172 Compare February 14, 2025 13:02
Reordered arguments in the AppliesAttribute test case to match the updated constructor signature. This ensures consistency and prevents potential test failures.
@carlosas
Copy link
Owner

PHP-CS-Fixer seems to want just this in AppliesAttributeTest.php :

/**
 * @coversNothing
 */

https://cs.symfony.com/doc/rules/php_unit/php_unit_test_class_requires_covers.html

Ensure tests are skipped for PHP versions below 8.0 using `setUp`. This prevents unsupported functionality from running and ensures consistent behavior across different environments.
@raffaelecarelle raffaelecarelle force-pushed the AppliesAttributeWithArgumentsRule branch from 27ee581 to f3c6868 Compare February 14, 2025 13:22
Added @Covers annotations for AppliesAttribute::getName and AppliesAttribute::matches methods to improve test coverage clarity. This ensures better tracking of what methods are being explicitly tested.
@raffaelecarelle raffaelecarelle force-pushed the AppliesAttributeWithArgumentsRule branch 2 times, most recently from 29ea06b to b4e169e Compare February 14, 2025 13:30
Replaced PHPUnit attributes with a data provider in `AppliesAttributeTest`. This improves readability and centralizes
@raffaelecarelle raffaelecarelle force-pushed the AppliesAttributeWithArgumentsRule branch from b4e169e to b75b60a Compare February 14, 2025 13:34
@raffaelecarelle
Copy link
Contributor Author

@carlosas, here we go :)

@carlosas
Copy link
Owner

Good job. However I'm testing manually and found another issue. When NOT using named arguments in the Attribute application, the test fails.

#[SimpleAttribute(something: 'something')]
class FixtureClass extends SimpleAbstractClass implements SimpleInterface

vs

#[SimpleAttribute('something')]
class FixtureClass extends SimpleAbstractClass implements SimpleInterface

is it because of the test or because of the feature?

@carlosas
Copy link
Owner

Since I merged the other PR, remember to merge/rebase the latest master 🙂 almost there!

@raffaelecarelle
Copy link
Contributor Author

Good job. However I'm testing manually and found another issue. When NOT using named arguments in the Attribute application, the test fails.

#[SimpleAttribute(something: 'something')]
class FixtureClass extends SimpleAbstractClass implements SimpleInterface

vs

#[SimpleAttribute('something')]
class FixtureClass extends SimpleAbstractClass implements SimpleInterface

is it because of the test or because of the feature?

Hi @carlosas, as I said on PR description, this is a feature limitation for now

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