[style] Expose computed vars for operators + arguments of expressions #307
Conversation
nishant-karajgikar
changed the title
nishant-karajgikar
force-pushed
the
nishantk/expressions-public-elements
branch
from
34f013b
to
8c715be
Compare
nishant-karajgikar
added
the
feature 🍏
nishant-karajgikar
force-pushed
the
nishantk/expressions-public-elements
branch
from
8c715be
to
8dbe459
Compare
nishant-karajgikar
force-pushed
the
nishantk/expressions-public-elements
branch
from
0a265ab
to
12a621e
Compare
| /// The operator of this expression | ||
| public var `operator`: Operator { | ||
| guard let first = elements.first, case Element.operator(let op) = first else { | ||
| fatalError("First element of the expression is not an operator.") |
There was a problem hiding this comment.
If it’s possible to create a malformed expression like this, consider adding a test that this error is thrown, so it doesn’t accidentally fall out during a refactor down the road.
There was a problem hiding this comment.
Ah it's not actually possible to do that.. I'm just fatally erroring to avoid making this return an optional.
| } | ||
|
|
||
| /// Initialize an expression with an operator and arguments | ||
| public init(operator op: Operator, arguments: [Argument] = [.null]) { |
There was a problem hiding this comment.
Does the default of [.null] cause the arguments getter to return a non-empty array?
There was a problem hiding this comment.
This was an oversight. It's been refactored out.
| @@ -112,6 +136,7 @@ public struct Expression: Codable, CustomStringConvertible, Equatable { | |||
| case string(String) | |||
| case boolean(Bool) | |||
| case numberArray([Double]) | |||
| case stringArray([String]) | |||
There was a problem hiding this comment.
Does the style specification allow for other types to occur inside a literal array, such as an array of booleans or objects, or even heterogeneous arrays (which are allowed in JSON)? What if, instead of separate cases for each type that could occur inside an array, we have just a single array case that takes the evaluated type as an additional associated value?
There was a problem hiding this comment.
Yeah, i've considered doing this... It's just that modeling those kind of heterogenous arrays is hard. Open to suggestions here.
nishant-karajgikar
force-pushed
the
nishantk/expressions-public-elements
branch
from
e3121c4
to
9be721b
Compare
| } | ||
| } | ||
|
|
||
| extension Array: ExpressionArgumentConvertible where Element == Double { |
There was a problem hiding this comment.
Does string array need to be here too?
There was a problem hiding this comment.
yep it does! Good catch.
| XCTAssertEqual(sumExp.operator, .sum) | ||
| XCTAssertEqual(sumExp.arguments.count, 2) | ||
| XCTAssertEqual(sumExp.arguments[0], .number(10)) | ||
| XCTAssertEqual(sumExp.arguments[1], .number(12)) |
nishant-karajgikar
force-pushed
the
nishantk/expressions-public-elements
branch
from
faad6f4
to
5c5388e
Compare
…#307) * Expose computed vars for operators + arguments of expressions * Support string based arrays as valid expression argument * Refactor ExpressionBuilder * review comment * cleanup Remove check for iOS 11 Update LogoView to make class and properties internal Update OrnamentsManager.options documentation Updated the changelog Remove options parameter from MapConfig.setupOrnaments() Updated changelog to note that MapView.update no longer used for ornaments
…#307) * Expose computed vars for operators + arguments of expressions * Support string based arrays as valid expression argument * Refactor ExpressionBuilder * review comment * cleanup
…#307) * Expose computed vars for operators + arguments of expressions * Support string based arrays as valid expression argument * Refactor ExpressionBuilder * review comment * cleanup Remove check for iOS 11 Update LogoView to make class and properties internal Update OrnamentsManager.options documentation Updated the changelog Remove options parameter from MapConfig.setupOrnaments() Updated changelog to note that MapView.update no longer used for ornaments
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: #299
Pull request checklist:
mapbox-maps-ioschangelog:<changelog> Expose the operator and arguments of an expression </changelog>.Summary of changes
operatorandargumentsfor an expression.