Convert RankedValue int ranks to Enum. #9526
Conversation
|
Depends on #9525 being merged to pass lint shard. |
|
|
||
| _rank: int | ||
|
|
||
| def __new__(cls, rank: int, display: str) -> "Rank": |
There was a problem hiding this comment.
Is your thinking that the complication of providing the display string explicitly is worth it so we can separate the symbolic value from the rank?
There was a problem hiding this comment.
No real thinking. I was working backwards from IntEnum not being a solution since options parsing expects Enum values to be strings. We can't simply switch that code to use Enum names (I naively tried) since we have Enum options that have names different from values. Since having a decoupling between Enum name and the value used in user-facing choices and display seems reasonable, I worked with the status quo.
There was a problem hiding this comment.
+1 on removing the display name from the value, if possible.
You could get the same display name by calling rank.name. rank.value will give you the int code.
There was a problem hiding this comment.
See my explanation to Benjy.
Ah, my GitHub UI was showing the original unedited comment without the full explanation when I wrote this. The explanation sounds reasonable. Thanks!
| from typing import Any, Dict, Iterator, List, Union | ||
|
|
||
|
|
||
| @total_ordering |
There was a problem hiding this comment.
With @total_ordering, you should also define __eq__: https://docs.python.org/3/library/functools.html#functools.total_ordering
Are you relying on the superclass's implementation? If so, consider adding this:
def __eq__(other):
return super().__eq__(other)While repetitive, it does make it much more explicit that you're meeting the two requirements of @total_ordering.
There was a problem hiding this comment.
I think @dataclass is explicit enough, don't you?
There was a problem hiding this comment.
I don't follow. You're not using @dataclass here and are using Enum.
If we were using dataclass, the solution would be @dataclass(order=True).
There was a problem hiding this comment.
Oops, yeah. But the exact same point applies to an Enum more strongly. By the very nature of a sealed immutable type equal, etc must work right. This is a language independent observation, you don't even need to know Python to assume it.
There was a problem hiding this comment.
Agreed, I don't see the value in explicit redundant boilerplate.
|
|
||
| _rank: int | ||
|
|
||
| def __new__(cls, rank: int, display: str) -> "Rank": |
There was a problem hiding this comment.
+1 on removing the display name from the value, if possible.
You could get the same display name by calling rank.name. rank.value will give you the int code.
See my explanation to Benjy. It's not possible without changing generic option parsing code. Further, we do not follow this advice for our "normal" enums where we should be using |
[ci skip-rust-tests] [ci skip-jvm-tests]
jsirois
force-pushed
the
options/rank/enum
branch
from
c1c6a2a to
dade270
Compare
|
Re-based to pick up |
My comment was actually in favor of what this change does. It seems important to distinguish the symbolic value from the rank (which I see has auxiliary information attached to the value). For example, you could imagine changing the rank if we decided for some reason that env vars should be higher in rank than flags. We definitely wouldn't want to change the value in that case. Granted, not a likely case, but it's helpful for working through the design. |
[ci skip-rust-tests]
[ci skip-jvm-tests]