search

Crell / enum-comparison

A comparison of enumerations and similar features in different languages
82 stars 7 forks source link

TARGET_ENUM/TARGET_CASE #36

Closed iluuu1994 closed 3 years ago

iluuu1994 commented 3 years ago

Like with reflection I don't think we should differentiate between TARGET_CLASS and TARGET_ENUM/TARGET_CASE. For example, we don't do that for interfaces/traits either.

Crell commented 3 years ago

You don't think there will be cases where you want an attribute that makes sense only on a Case?

iluuu1994 commented 3 years ago

Not more than an attribute just for interfaces or traits, I guess. At the end of the day you can still check that the given class is an enum or case through reflection. @beberlei Any thoughts on this?

beberlei commented 3 years ago

@Crell @iluuu1994 do you have an actual use case of the top of your head where an attribute can realistically only be on an enum vs a class? I don't. I wouldn't differentiate this right now, keeps it simpler.

iluuu1994 commented 3 years ago

No, I can't think of any. We had the #[CaseValue] use case but this becomes unnecessary with our dedicated primitive value syntax. IMO class is enough for enums and const is enough for cases. @Crell Can you think of any?

Crell commented 3 years ago

How about we add it to the open questions section and poll the audience. I feel like cutting off the ability to say "this only makes sense on enums" just because we can't think of one is too hubristic.

iluuu1994 commented 3 years ago

I'd be ok with that. Note that implementing this behavior is still entirely possible, it just requires a manual $reflectionClass->isEnum() or $reflectionClassConstant->isEnumCase() check.

iluuu1994 commented 3 years ago

@Crell We need to make some decisions. I'm against this feature mostly because I don't see the value at this point. You can also easily check that the given class is an enum with RefectionClass::isEnum() (this is actually undocumented, we'll need to specify this in the RFC). But I will also not fight you if you insist on this feature.

beberlei commented 3 years ago

You should really remove this, the reflection part is already the tricky issue about the whole proposal. Adding even more complexity here may tilt opinion towards no in my opinion

iluuu1994 commented 3 years ago

Removed from the RFC.

Crell commented 3 years ago

After further discussion we decided to drop the extra targets, but I reserve the right to say "I told you so" to both of you if use cases where a class/const attribute is not appropriate on an enum/case or vice versa is found in the future. :stuck_out_tongue: