Clean up some LLVM code - #16168
Conversation
There is no sense in exposing that we are returning a list. The list should be immutable after this function call anyway. Signed-off-by: Tristan Partin <tristan@partin.io>
We don't extend the list later, so let's use a tuple to make it immutable. Signed-off-by: Tristan Partin <tristan@partin.io>
Signed-off-by: Tristan Partin <tristan@partin.io>
eli-schwartz
left a comment
eli-schwartz
left a comment
There was a problem hiding this comment.
The overall goal here is not obvious but the implementation is definitely not something I can live with.
| return None | ||
|
|
||
| def get_llvm_tool_names(tool: str) -> T.List[str]: | ||
| def get_llvm_tool_names(tool: str) -> T.Iterable[str]: |
There was a problem hiding this comment.
No, no, no, absolutely not.
There is no sense in exposing that we are returning a list. The list
should be immutable after this function call anyway.
This is type pedantry of the most impractical sense. str is the same type-checking type as Iterable[str].
Do not use "Iterable" or "Sequence" or related type concepts if one of the allowable types is a string. It gleefully type-checks the wrong thing.
The type system is broken. Opting out of the parts of it that are broken is the only valid solution. There's no such thing as a bug caused by "exposing that we are returning it as a list".
2 participants
No description provided.