Allow class inheritance determination for enums#16
Allow class inheritance determination for enums#16agustingomes wants to merge 1 commit intoCrell:masterfrom
Conversation
agustingomes
force-pushed
the
enum-implementing-interface
branch
from
c3798ff to
e949de1
Compare
agustingomes
force-pushed
the
enum-implementing-interface
branch
from
e949de1 to
93f1875
Compare
|
@Crell Thank you for the review and the suggestions. I applied them. |
README.md
Outdated
| use Crell\AttributeUtils\Inheritable; | ||
|
|
||
| #[Attribute(Attribute::TARGET_CLASS)] | ||
| final readonly class TypeMap implements Inheritable |
There was a problem hiding this comment.
Serde TypeMaps were the reason I added this feature, but I don't think they make a particularly good example. It requires a lot of context to understand why you'd want to do it. For the README, we should have a much simpler and more understandable example. Remember, not everyone using AU is using Serde. (I hope... 😄 )
There was a problem hiding this comment.
Indeed, that is a reasonable take. I used the Serde TypeMaps since that is what I'm using as reference for these changes.
The TypeMap also helped me learn that if an attribute does not implement Inheritable the result is different than expected, which is something I believe the Readme needs to account for.
With that said, I will try to write an example that is not tied to the TypeMaps.
There was a problem hiding this comment.
Hopefully the new example I pushed makes more sense and requires less context.
src/AttributeParser.php
Outdated
| * @return iterable<\ReflectionClass<object>> | ||
| * @throws \ReflectionException | ||
| */ | ||
| protected function enumInheritanceTree(\ReflectionEnum $subject): iterable |
There was a problem hiding this comment.
Am I missing something, or is this identical to classInheritanceTree()? If they're the same, there's no reason to duplicate it. It's fine to have attributeInheritanceTree() call the same method for classes and enums, since enums are classes.
There was a problem hiding this comment.
It is indeed identical.
In my first iteration, I did use classInheritanceTree method instead, but when resolving the PHPStan issues it raised, it felt like a better approach to implement a specific method for ReflectionEnum.
I will take a second look at the PHPStan issues and see if I can figure them out, because the enum are classes under the hood, so it definitely makes sense to re-use the existing classInheritanceTree.
There was a problem hiding this comment.
Fixed. The annotation change ended up being simpler. Although I'd have expected ReflectionEnum to be accepted as ReflectionObject is, but maybe because of the UnitEnum, it was not accepted the same way.
### Motivation In the scope of [a downstream project issue](Crell/Serde#10), It was noticed that in cases where an enum implemented an interface, the interface attributes could not be read. This change allows `ReflectionEnum` to retrieve such information if it exists
agustingomes
force-pushed
the
enum-implementing-interface
branch
from
93f1875 to
829d9cc
Compare
2 participants
Description
Read enum implemented interfaces and its attributes
Motivation and context
In the scope of a downstream project issue, It was noticed that in cases where an enum implemented an interface, the interface attributes could not be read.
This change allows
ReflectionEnumto retrieve such information if it exists.How has this been tested?
Unit test case has been added to existing test
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist:
Go over all the following points, and put an
xin all the boxes that apply.Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests and code style pass.