Closed njlr closed 1 year ago
There the dict
decoder/encoder which support Map<string, 'T>
.
I am unsure why I didn't make the key generics at the time too. I see two potential reason:
dict
was implemented like that in the Elm library and I didn't think more about itDepending on the answer, there are 2 solutions:
dict
accept generics for both the keys and the valuesAh, I think I skipped over dict
assuming it was for IDictionary
. I think the ability to have non-string keys is quite important; this is a key advantage of Fable over JavaScript for me.
Happy to send a PR with some direction :+1:
Ah, I think I skipped over dict assuming it was for IDictionary.
Yes, you are not the first one. That's the problem when you have a type Map
and the function map
to support. 😅
Happy to send a PR with some direction 👍
The first thing is to decide how we are going to represent the type as JSON. I think you proposed to use an array representation with the first value being the key and the second the value. I think this is indeed the only way we have to do it.
Then for the actual code changes:
Rewrite keyValuesPairs
to accept a keyDecoder
and valueDecoder
.
This is because it allows to have a really simple implementation for the dict
decoder.
Which would also need to be changed to accept a keyDecoder
and valueDecoder
.
There is also the Auto
API to cover:
I think the only changes required is to convert typedefof< Map<string, obj> >.FullName
to typedefof< Map<obj, obj> >.FullName
because the keyDecoder
is already auto generating its type.
The same things needs to be done for the encoders and the tests needs to be adapt :). Then we need to port the change to Thoth.Json.Net
but in general this is just a matter of copy/pasting the code.
Because, we are going to make a breaking change to the API by adding an arguments to dict
, I would like to take this opportunity to change dict
to returns the F# dictionary type (is it IDictionary
? Is it supported by Fable?).
And then find a good name for the Map
type.
Some proposition I have are:
Decode.Map
, which a capital letter to not conflict with map
--> too subtle ?Decode.asMap
-> Doesn't follow the same convention as the other decoderDecode.toMap
-> Doesn't follow the same convention as the other decoderDecode.mapping
looks a bit strange to meDecode.map'
not perfect but "ok" ?Personally, my preference goes to Decode.map'
Decode.keyValueListMap
is verbose, but descriptive. It also gives a clue as to how the map will be encoded as a JSON array.
If breaking changes are on the table, another option might be:
Decode.map
Function to create a Decoder<Map<'k, 'v>>
Decoder.map
Function to map the result of a successful decoding (currently Decode.map
)I like Decoder.map
because Decoder
is a module of functions over decoders, which mirrors List.map
, Seq.map
, etc.
Although this could be extremely confusing for existing users! It is also a further departure from Elm.
Decode.keyValueListMap
is verbose, but descriptive. It also gives a clue as to how the map will be encoded as a JSON array.
Indeed, it is verbose but if I was to go this way I would probably use Decode.keyValueListAsMap
or Decode.keyValueListToMap
.
I like
Decoder.map
because Decoder is a module of functions over decoders, which mirrorsList.map
,Seq.map
, etc.
For now, I think this is too much of a breaking change I think.
I still have the hope to rewrite Thoth.Json from scratch in the future and perhaps it will be a better time to have such a big breaking changes.
It is also a further departure from Elm.
This is not something holding me personally. I mean we already have some specific syntax thanks to classes and reflection support (even if I still think reflection support is an error ^^)
PR for the most minimal version of this feature: https://github.com/thoth-org/Thoth.Json/pull/136
Decode.dict
is unchangedDecode.map'
is addedDecode.Auto
supports maps with non-string keysWill be included in the next release
I was wondering if the library should have built-in support for maps, similar to lists and sequences?
(
Decode.map
is already taken of course)Perhaps...