Conversation
flobernd
force-pushed
the
ordered_dictionary
branch
from
e61cc72 to
829202e
Compare
|
as reported in elastic/elasticsearch-java#932 |
flobernd
force-pushed
the
ordered_dictionary
branch
from
829202e to
94a2249
Compare
| context.push(`Property '${prop.name}'`) | ||
|
|
||
| if (prop.type.kind === 'dictionary_of' && prop.type.ordered) { | ||
| if (prop.name !== 'aggregations') { |
There was a problem hiding this comment.
This check only works for the "top-level" type of properties and does not check the type-name, but I think this is sufficient for now. WDYT?
There was a problem hiding this comment.
I guess this is to catch the many occurrences of aggregations as a property name?
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
There was a problem hiding this comment.
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
Correct!
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
I don't think there is a good way to detect this using a pattern. We have also cases like this:
Dictionary<string, ApiKeyAggregationContainer>where we define special containers to restrict the valid aggregations to a subset.
swallez
left a comment
swallez
left a comment
There was a problem hiding this comment.
We should be more assertive in the usage warning. Also left a few comments.
| key: ValueOf | ||
| value: ValueOf | ||
| singleKey: boolean | ||
| ordered: boolean |
There was a problem hiding this comment.
Side comment: interesting case of a boolean property (singleKey) that evolves into an enum (singleKey, ordered, unordered) as ordered only makes sense when singleKey == false.
We can however keep it as two properties:
- some languages have their default dictionary keeping order (e.g. PHP, Ruby, JS) and their generators can safely ignore
orderedbecause it makes no difference to them, - some languages don't have ordered dictionaries (e.g. Go) and their generators will therefore also ignore
ordered.
There was a problem hiding this comment.
Yes, I was thinking about an enum, but I as well didn't want to break all existing tooling 😅
|
|
||
| if (typeof tags.es_quirk === 'string') { | ||
| type.esQuirk = tags.es_quirk | ||
| type.esQuirk = tags.es_quirk.replace(/\r/g, '') |
There was a problem hiding this comment.
Yes 😵💫 I had applied this to most other places a while ago, but seems like I forgot about the quirks. This will ensure deterministic output regardless of being generated on *nix or Windows.
| context.push(`Property '${prop.name}'`) | ||
|
|
||
| if (prop.type.kind === 'dictionary_of' && prop.type.ordered) { | ||
| if (prop.name !== 'aggregations') { |
There was a problem hiding this comment.
I guess this is to catch the many occurrences of aggregations as a property name?
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
|
|
||
| export class SingleKeyDictionary<TKey, TValue> {} | ||
|
|
||
| export class OrderedDictionary<TKey, TValue> {} |
There was a problem hiding this comment.
We should restate the warning here, for people who will discover it in the API spec code.
swallez
left a comment
swallez
left a comment
There was a problem hiding this comment.
We also have another occurrence of this in elastic/elasticsearch-java#99 - Pivot.group_config should be ordered (and added in the validation):
3 participants
Adds a new builtin dictionary type
OrderedDictionary<K, V>that can be used instead ofDictionary<K, V>when the order of items is important.Motivation
Some dictionaries require a fixed order (e.g.
aggregations), but currently we do not model this fact at all.This is quite special since the JSON spec does not make any guarantees about the order of properties in an object, but still Elasticsearch relies on that.
Closes: #3908
Related to: #3909