Custom AttributeOptions #748
Conversation
For reference, this is tracked in #718 as a future backwards-incompatible change. |
| @@ -122,3 +135,101 @@ extension AttributeOptions: Codable { | |||
| self = AttributeOptions(descriptions: descriptions)! | |||
| } | |||
| } | |||
|
|
|||
| extension AttributeOptions { | |||
There was a problem hiding this comment.
This is a pretty extensive extension that makes #718 all the more desirable. That said, the SetAlgebra protocol that OptionSet conforms to requires several other methods that OptionSet is currently implementing, now incompletely. The fundamental methods seem to be ==, intersection(_:), and union(_:), with all the others being derived from them. This is a different set of methods than what this extension implements, so I’m not sure if there would be inconsistencies as a result.
There was a problem hiding this comment.
Right. What I wanted to discuss before fully committing to the impl. is if this approach is really worth the result. I am inclined to call it over-engineered and potentially confusing to use, but it gets the job done in a nonbreaking way.
So do we need to allow options customizing even in suboptimal way now or we are OK to wait till it is time for API breakage update?
There was a problem hiding this comment.
I guess my question was whether a different subset of methods (==, intersection(_:), and union(_:)) could be overridden to ensure that all the SetAlgebra methods works, since we can’t restrict which of those methods a developer will use in their code. If not, I agree that having to reimplement all the SetAlgebra methods just for AttributeOptions would be overkill. But our current use of OptionSet for RoadClasses similarly blocks further support for the Directions API, such as #662, so if the solution is general enough to cover both types, I think the additional code would be worth the trouble.
Udumft
force-pushed
the
vk/NAVIOS-374-attribute-options
branch
from
7f8aef6
to
e3d946a
Compare
| /// [1: "value1"] != [1: "value2"] | ||
| /// [1: "value1"] != [:] | ||
| /// [:] == [:] | ||
| case allowEqual |
There was a problem hiding this comment.
Not sure about the naming at this point...
There was a problem hiding this comment.
Just equal is fine, with precedent in MPSNNComparisonType.comparisonType, NSSpecifierTest.TestComparisonOperation, and BNNS.RelationalOperator.
| /// [1: "value1"] != [1: "value2"] | ||
| /// [1: "value1"] != [:] | ||
| /// [:] == [:] | ||
| case allowEqual |
There was a problem hiding this comment.
Just equal is fine, with precedent in MPSNNComparisonType.comparisonType, NSSpecifierTest.TestComparisonOperation, and BNNS.RelationalOperator.
… and manipulation methods to allow custom options to be processed.
…ase for AttributeOptions and other similar types to allow custom options to be set.
Udumft
force-pushed
the
vk/NAVIOS-374-attribute-options
branch
from
fcae58c
to
8a54bd9
Compare
Added
AttributeOptions.customOptionsand manipulation methods to allow custom options to be processed.This is an approach to allow customization while avoiding breaking changes. The safer option would be to refactor AttributeOptions into a struct.