Resolve NumberType | NumberArrayType ambiguity with a new SizeType #15818
Conversation
|
📦 Preview the website for this branch here: https://deploy-preview-15818--ol-site.netlify.app/. |
ahocevar
force-pushed
the
size-type
branch
4 times, most recently
from
9a4f459
to
f356c9f
Compare
| 'circle-scale': ['get', 'iconSize', 'number[]'], | ||
| 'circle-scale': ['get', 'iconSize'], |
There was a problem hiding this comment.
This is the actual achievement - no type hint needed any more!
|
This is now ready for review. |
| @@ -485,6 +497,12 @@ function compile(expression, returnType, context) { | |||
| return arrayToGlsl(/** @type {Array<number>} */ (expression.value)); | |||
| } | |||
|
|
|||
| if ((expression.type & SizeType) > 0) { | |||
There was a problem hiding this comment.
Since anything that includes the NumberType is already handled above, isn’t this only testing that NumberArrayType is included?
I’m only reviewing on a small device, so probably missing context, but it isn’t clear to me why we aren’t checking for more narrow types in all these ifs.
There was a problem hiding this comment.
Oh, I must be missing something. I see NumberArrayType is also checked right above.
There was a problem hiding this comment.
@tschaub Looks like you're not missing anything, but that code is missing tests. Looking into it now.
There was a problem hiding this comment.
I think SizeType should be a distinct type, like ColorType, instead of a broadened type. But I haven't been able to fully wrap my head around the types involved in the parsing and compilation phases to figure out how that works. So @tschaub, if you have an idea how that could be achieved, any help would be appreciated.
There was a problem hiding this comment.
Nevermind, I think I figured it out, @tschaub. Before I commit, I'm adding more tests.
ahocevar
force-pushed
the
size-type
branch
3 times, most recently
from
a57c160
to
e10117a
Compare
|
@tschaub Ready for review - tests added, |
This pull request removes the only real-life need for type hints in
"get"call expressions, which was needed due to an ambiguity betweenNumberTypeandNumberArrayTypefor-scalestyle properties.With the new
SizeType, it would also be possible to support styles like'icon-size': 24instead of'icon-size': [24, 24], but I did not make this change here.