Talk:Requests for comment/Typesafe enums

Looking forward to using it!

That's a really solid proposal and code. The only suggestions I have are:

• Don't worry about the "final" class tags, it doesn't really belong in the reference implementation. Developers know they can use final if they really want, and it's a bit distracting from your point.
• Maybe add more basic language constructs to the unit tests to dispel any potential anxiety. For example, in_array(), array_search(), and switch.

Can I also say, that is some really clean code? Adamw (talk) 21:26, 16 May 2014 (UTC)

Done, and done. And thanks! AGreen (WMF) (talk) 18:01, 18 May 2014 (UTC)

Database Safety

Since there is no guarantee that the internal values will remain the same, how would one store an enum value in a database? Doing so as a string is not desirable since string indicies are large and inefficient compared to integer indicies. (I think what I'm trying to get at is that there should be some way of explicitly specifying an integer per enum 'constant'.) Mwalker (WMF) (talk) 23:00, 16 May 2014 (UTC)

Yeah, that makes a lot of sense! I'm sure it's doable, I think we just have to figure out the best way to do it. Here are some considerations: the definition and usage should be compact and intuitive, and it should be coherent with and/or reuse other more general facilities that we may add.

Also, if you have a some specific use cases, just to have some concrete example code to stare at, that'd be great. Thanks! AGreen (WMF) (talk) 18:01, 18 May 2014 (UTC)

Bitmask Enums

Sometimes it's nice to have a bitmask as an enum; it would be really cool if those could be automatically generated too in some sort of database safe fashion. Mwalker (WMF) (talk) 23:29, 16 May 2014 (UTC)

Agreed. See my response under #Database Safety, I'd say basically the same thing on this. AGreen (WMF) (talk) 18:01, 18 May 2014 (UTC)

RFC review 2014-05-21

Architecture_meetings/RFC_review_2014-05-21

tl;dr Will benchmark setup and access. No objections noted to the features requested above. Possible additional use cases mentioned. AGreen (WMF) (talk) 02:59, 22 May 2014 (UTC)

Violates PSR-1

The call to setUp() can go in the same .php file as the enum definition.

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md 3rd point

--Jeroen De Dauw (talk) 12:30, 22 May 2014‎ (UTC)

This is true, at least technically. The standard takes "side effects" to include "modifying global or static variables". The call to setUp() sets the values of the class's public static properties and adds to a private static index in the abstract superclass.

However, I don't think it violates the spirit of that standard. The call to setUp() just prepares the class to be used as expected. It doesn't output anything, access services or modify application state outside its own class hierarchy. It's the equivalent of a simple static initialization block. It does need to be called right away, before any other code tries to use the enums.

Digging a bit deeper, I came across one of the reasons for the "no side effects" stipulation in Larry Garfield's post on this thread: in brief, he says it's to avoid having a file with both symbols (which means it can only be included once) and logic (which you might want to call more than once, depending on the circumstances).

In this case, a single call to setUp() is not a significant amount of logic that would ever be any trouble to repeat anywhere else.

(Also: I just uploaded a new version of the proposed implementation that allows setUp() to be called any number of times. This doesn't impact much on the issue, just makes the implementation a bit more flexible and eliminates an error condition.)

If the consensus is that we shouldn't violate PSR-1 even in this manner, there are still three other options:

1. Modify autoloading to make it possible to set an "initialization" method to be called when a class is loaded. (Disadvantage: it would make the code less transparent.)
2. Require all code that uses enums to call setUp() to make sure they're... set up. (Disadvantage: verbose.)
3. Use the alternate method-based approach that I've just added to the RFC. (Disadvantage: it's ugly. Think MyEnumClass::THIS_IS_MY_UGLY_ENUM(). Bleah.)

That said, it's clear that the setUp() call is the most unpretty part of this. Still, I think the benefits outweigh the drawbacks.

-- AGreen (WMF) (talk) 05:23, 25 May 2014 (UTC)

There's lots of static code—isn't that bad?

One comment faults the proposed implementation for having “lots of static code”.

However, it would be odd and not worth the extra complexity to emulate enums along these lines without static code. This is borne out by the fact that several other implementations out there also have plenty of static code.

There are indeed times when it's better to avoid static code. But that's not the case here. This is why:

• Testability isn't affected.
  • Static code can make testing more difficult. However, the proposed implementation includes unit tests with full coverage.
  • In most cases, there will be no additional logic in the concrete enum classes, so there will be nothing to test there. However, if you did include extra logic in a concrete enum class, it would not be hard to test.
  • In tests of other classes that make use of enums, it's possible to mock the enums in tests of methods that don't contain references to specific enum members.
  • It wouldn't be possible to switch in mocks for references to specific enum members. This is about the only limitation on testability, as far as I can tell. Note that the alternate approach that I've added to the RFC would allow substituting specific enum members, but really I don't think such mocks should be necessary.
• Global access is desired.
  • It's true that each enum is a global access point. That's also true of class constants, and it's the way enums usually work. It's part of what makes them concise and useful.
• Better non-static patterns aren't available.
  • Sometimes avoiding static code leads to better overall OO design. But in this case, at least as far as I know, there are no better OO enum patterns that avoiding static code could lead to.

-- AGreen (WMF) (talk) 05:23, 25 May 2014 (UTC)

My impression after a quick look

If I had to place a bet on this making code worse or better, it'd unforunately be the fomer. The implementation is rather suspect with the PSR-1 violation and lots of static code. This also reminds me of the GenericArrayObject class I created and put in core, which is not a good thing. What I'd recommend here is that you ask knowledgeable people outside of the MediaWiki community what they think about it. For instance, start a topic on the PHP-FIG list.

--Jeroen De Dauw (talk) 12:37, 22 May 2014 (UTC)

Thank you very much for taking a look! I've modified the implementation and the RFC following this. I've also replied in detail in other sections.

Regarding getting advice from the broader community: sounds good. A wide field of opinions can be really fantastic.

In addition to my replies in other sections, I'd like to make a general point that I'm sure you're aware of, but I think is worth mentioning: many, maybe all, rules are leaky abstractions. Good applications of them should often recall the reasoning behind them. Also: one of the functions of code is communication between people. Great code is an easy read. This may also be held in the balance, together with other issues, when we think about code quality.

Thanks again!

--AGreen (WMF) (talk) 05:23, 25 May 2014 (UTC)

Happy to see you are holding readability and things such as leaky abstractions into account! May I suggest to implement this in a standalone library, and use it on a smaller scale at first? If that happens I'll happily have a closer look at it. This approach avoids the risk of adding yet another thing into the big ball of mud that is MediaWiki, after which it is hard to get out or modify, when it turns out you made a mistake. (Sigh, that lack of boundaries...)

-- Jeroen De Dauw (talk) 21:02, 12 August 2014 (UTC)