Get rid of instanceof in StringOrArray extension#163
Get rid of instanceof in StringOrArray extension#163herndlm wants to merge 1 commit intoszepeviktor:masterfrom
Conversation
|
I'm not quite sure that this is sufficient. While |
|
Not sure tbh, this does not change behaviour at least to before, except e.g. supporting unions of strings too. Maybes like mixed were also not considered before. It could still be improved any time I guess |
| if ($keyType->isString()->yes()) { | ||
| return new ArrayType(new StringType(), new StringType()); | ||
| } | ||
| return new ArrayType(new IntegerType(), new StringType()); |
There was a problem hiding this comment.
| return new ArrayType(new IntegerType(), new StringType()); | |
| if ($keyType->isString()->no()) { | |
| return new ArrayType(new IntegerType(), new StringType()); | |
| } | |
| return TypeCombinator::union( | |
| new ArrayType(new StringType(), new StringType()), | |
| new ArrayType(new IntegerType(), new StringType()) | |
| ); |
There was a problem hiding this comment.
I see what you're going for I think. I don't really understand though what this extension is even doing :) I was just replacing stuff without changing much behaviour.
Why does it not just pass through the array but recreate it for example. Unfortunately not on my computer anymore to dig deeper..
There was a problem hiding this comment.
It sets the value type to string no matter what. However, this is not what the extension was intended to I think. See #154.
There was a problem hiding this comment.
I'll check it later and try to add tests too it not existing yet. But does it make sense to you to directly pass through the key type? It should noy be modified, right?
There was a problem hiding this comment.
I have an idea, give me a bit of time :)
|
I agree, it is as good as before, it just makes it more obvious that the unknown key type case was not covered. |
2 participants
No description provided.