[LTB-61] Add ToJSON instance for ConfigRec

[gpevnev]

Problem: There is no way to display partial or final config.

Solution: Implement ToJSON instance for ConfigRec

[gpevnev]

@kirelagin There are no tests for now, as I am still thinking about better way to do this.

Firstly I don't like that encode . decode = id property doesn't hold now for ConfigRec JSON insances because of <undefined> placeholders on missing options. I think that some kind of functionality like this would be useful for debugging.

So one way I thought about solving this problem is creating a newtype and writing a separate instance for it, which supports reporting <undefined> options.

@kirelagin do you have a better proposal?

[gpevnev]

Also @kirelagin I was thinking that it would be cool if we keep some information about sources of different options in final config and present it if necessary, so users wouldn't need to manually debug and track exact place where an option came from

[gpevnev]

This might be implemented semantically wrong, because I haven't understood meaning of this

[kirelagin]

I have no idea either 😅. I believe, @pasqu4le added sum types.

[pasqu4le]

Yep, I added support for sum types, although quite some time ago, so I had to check the code again because I forgot how 😅

Anyway, this seems to be semantically correct.

If the confusion is about why there are 2 ItemSum I am not entirely sure, I recall something about them requiring different constraints but I cannot find this now and judging by some comments in the PR this difference might have been solved at some point.

In other terms, unless I am missing something, it looks like we could have had a single:

    ItemSum       :: Item' k        (l ::+ is) -> Item k        (l ::+ is)

just like we have a single ItemSub and the implementation here is indeed correct.

P.s. I could check this for sure and open up another PR to fix this if necessary

[kirelagin]

<undefined>

Hmmm, I guess I don’t understand something. If the config is “final”, then there can be do undefined options in it. If the config is “partial”, then you just don’t add undefined options to the resulting JSON object. Isn’t that right?

[pasqu4le]

If the config is “final”, then there can be do undefined options in it

That's not entirely true for sum types, because basically they are implemented with product types you end up with "undefined" branches, which is to say equivalent of non-used constructors of a sum-type.

For example this:

type ExampleParams =
   '[ "params" ::+
       '[ "basic" ::- OtherType
        , "advanced" ::- OtherType
        ]
    ]

could be parsed (and finalized) from this:

params:
  paramsType: "basic"
  basic:
    [... othertype ...]

and "advanced" would be "undefined".

[pasqu4le]

I am not sure I understand the need for the <undefined> placeholder, shouldn't it be just... not defined, like, not there at all?

Final configurations are complete, so the steps of: encode -> decode (to partial) -> finalise should produce the starting value (as id) without a problem if we just produce nothing in the JSON instance for undefined sum-type constructors (the only thing that can be non-defined in final configs).

The same can be said about partial configurations (except of course that finalise should be avoided), because non-defined part of it will be decoded as non-defined when they are not present in the JSON/YAML source, no?

[gpevnev]

@pasqu4le the whole purpose of this is to show configs to human and let them debug why config value is not which they expected. I added <undefined> string so people can easily see what is missing in that partial configuration and maybe it is not the best way to do this.
In general I think it might be cool if we can smoother UX even more if we can preserve info about value from which partial config ended in final and make "default" strategy for configuration even more simple.
For example add function which read config files from:

1. System wide config
2. User config in $HOME
3. Config options from env variables after Parse config from environmental variables #51 is merged
4. Config file from CLI options if specified
5. Config option from CLI

[pasqu4le]

Ok, so it seems to me we'd like to obtain two things:

• produce a valid ToJSON instance from config (that also respects the encode . decode = id property)
• obtain a textual representation for config that helps with debugging (by also reporting missing options)

Ideally we'd have a ToJSON instance that satisfies both, but I don't know how to do so either (if this was YAML only it would have been a great use of comments IMO, but sadly this won't work for JSON).

one way I thought about solving this problem is creating a newtype and writing a separate instance for it, which supports reporting options.

I suggest to go with this approach, it could be done fairly simply by adding a boolean argument to configToJsonList.

We define the OptionsToJsonList typeclass here anyway and we could have its function use the argument to decide whether to report <undefined> values or produce nothing at all for them.

Of course we can then have the

instance OptionsToJsonList k is => ToJSON (ConfigRec k is)

pass said argument already to obtain a valid, without <undefined>, JSON representation (that can be tested to hold the property).

As for the debugging-helpful representation, you can make a newtype for it and have its instance use configToJsonList with the opposite boolean argument.

[pasqu4le]

Regarding your ideas about sources I guess to be able to report where a configuration came from we'd need to store this information in Loot.Config.Record constructors, right?

TBH I don't know what's the current status of lootbox on the matter.

[gpevnev]

1. Made ToJSON instance respect decode . encode == id law.
2. Added Buldable instance for config.

@pasqu4le @kirelagin take a look please when you have time.
I still haven't decided on best way to display configs using Buildable, you can see example in test and please give your feedback on it

[pasqu4le]

This import list is odd, it goes outside the 80 line length limit and you use all the Item constructors anyway, so I suggest:

Suggested change

	import Loot.Config.Record ((:::), (::<), (::+), (::-), ConfigKind,
	ConfigRec, Item (ItemOptionP, ItemOptionF, ItemSub, ItemSumP, ItemSumF, ItemBranchP, ItemBranchF),
	ItemKind, SumSelection)
	import Loot.Config.Record ((:::), (::<), (::+), (::-), ConfigKind, ConfigRec,
	Item (..), ItemKind, SumSelection)

[pasqu4le]

fromString $ symbolVal (Proxy :: Proxy l) is used in several places, I suggest extracting it into a function.

[pasqu4le]

Overall LGTM, I think Buildable is the right choice as well, given that the class is supposed to be used to display things in a nice, readable, way.