-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Omit inferred type annotations for 'coalesce' arguments #5765
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,20 @@ class ParsingContext { | |
this.expectedType = expectedType; | ||
} | ||
|
||
parse(expr: mixed, index?: number, expectedType?: ?Type, bindings?: Array<[string, Expression]>): ?Expression { | ||
/** | ||
* @param expr the JSON expression to parse | ||
* @param index the optional argument index if this expression is an argument of a parent expression that's being parsed | ||
* @param options | ||
* @param options.omitTypeAnnotations set true to omit inferred type annotations. Caller beware: with this option set, the parsed expression's type will NOT satisfy `expectedType` if it would normally be wrapped in an inferred annotation. | ||
* @private | ||
*/ | ||
parse( | ||
expr: mixed, | ||
index?: number, | ||
expectedType?: ?Type, | ||
bindings?: Array<[string, Expression]>, | ||
options: {omitTypeAnnotations?: boolean} = {} | ||
): ?Expression { | ||
let context = this; | ||
if (index) { | ||
context = context.concat(index, expectedType, bindings); | ||
|
@@ -68,26 +81,29 @@ class ParsingContext { | |
if (Expr) { | ||
let parsed = Expr.parse(expr, context); | ||
if (!parsed) return null; | ||
const expected = context.expectedType; | ||
const actual = parsed.type; | ||
if (expected) { | ||
// When we expect a number, string, or boolean but have a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about: // When we expect a number, string, boolean, or array but
// have a Value, we can wrap it in a refining assertion.
// When we expect a Color but have a String or Value, we
// can wrap it in "to-color" coercion.
// Otherwise, we do static type-checking.
if ((expected.kind === 'string' || expected.kind === 'number' || expected.kind === 'boolean') && actual.kind === 'value') {
if (!options.omitTypeAnnotations) {
parsed = new Assertion(expected, [parsed]);
}
} else if (expected.kind === 'array' && actual.kind === 'value') {
if (!options.omitTypeAnnotations) {
parsed = new ArrayAssertion(expected, parsed);
}
} else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) {
if (!options.omitTypeAnnotations) {
parsed = new Coercion(expected, [parsed]);
}
} else if (context.checkSubtype(context.expectedType, parsed.type)) {
return null;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated with this approach. |
||
// Value, wrap it in a refining assertion, and when we expect | ||
// a Color but have a String or Value, wrap it in "to-color" | ||
// coercion. | ||
const canAssert = expected.kind === 'string' || | ||
expected.kind === 'number' || | ||
expected.kind === 'boolean'; | ||
|
||
if (canAssert && actual.kind === 'value') { | ||
parsed = new Assertion(expected, [parsed]); | ||
|
||
if (context.expectedType) { | ||
const expected = context.expectedType; | ||
const actual = parsed.type; | ||
|
||
// When we expect a number, string, boolean, or array but | ||
// have a Value, we can wrap it in a refining assertion. | ||
// When we expect a Color but have a String or Value, we | ||
// can wrap it in "to-color" coercion. | ||
// Otherwise, we do static type-checking. | ||
if ((expected.kind === 'string' || expected.kind === 'number' || expected.kind === 'boolean') && actual.kind === 'value') { | ||
if (!options.omitTypeAnnotations) { | ||
parsed = new Assertion(expected, [parsed]); | ||
} | ||
} else if (expected.kind === 'array' && actual.kind === 'value') { | ||
parsed = new ArrayAssertion(expected, parsed); | ||
if (!options.omitTypeAnnotations) { | ||
parsed = new ArrayAssertion(expected, parsed); | ||
} | ||
} else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) { | ||
parsed = new Coercion(expected, [parsed]); | ||
} | ||
|
||
if (context.checkSubtype(expected, parsed.type)) { | ||
if (!options.omitTypeAnnotations) { | ||
parsed = new Coercion(expected, [parsed]); | ||
} | ||
} else if (context.checkSubtype(context.expectedType, parsed.type)) { | ||
return null; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"propertySpec": {"type": "string"}, | ||
"expression": [ | ||
"coalesce", | ||
["get", "a"], | ||
5 | ||
], | ||
"expected": { | ||
"compiled": { | ||
"result": "error", | ||
"errors": [{ | ||
"key": "[2]", | ||
"error": "Expected string but found number instead." | ||
}] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
{ | ||
"propertySpec": {"type": "string"}, | ||
"expression": [ | ||
"coalesce", | ||
["get", "a"], | ||
["get", "b"] | ||
], | ||
"inputs": [ | ||
[{}, {"properties": {"a": "one"}}], | ||
[{}, {"properties": {"b": "two"}}], | ||
[{}, {"properties": {"b": 5}}], | ||
[{}, {"properties": {}}] | ||
], | ||
"expected": { | ||
"compiled": { | ||
"isFeatureConstant": false, | ||
"isZoomConstant": true, | ||
"result": "success", | ||
"type": "string" | ||
}, | ||
"outputs": [ | ||
"one", | ||
"two", | ||
{"error": "Expected value to be of type string, but found number instead."}, | ||
{"error": "Expected value to be of type string, but found null instead."} | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit isn't strictly necessary -- we could just always return
new Coalesce(ValueType, parsedArgs)
-- I figured including it would be more explicit & therefore clearer.