FsConfig icon indicating copy to clipboard operation
FsConfig copied to clipboard

Optional subsections does not work.

Open da9l opened this issue 4 years ago • 4 comments

Optional subsections does not work for .json files

type SubSection = {
    valueA : string
}

type Settings = {
   subSection : SubSection option
}

Reading a json with the subsection in place gives a null value.

da9l avatar Aug 14 '20 11:08 da9l

Hello @queil and @tamizhvendan! (I'm pinging you two since you seem to be the maintainers of this repo)

I had a quick stab at trying to fix/implement this, but I quickly got stuck. This is because the IConfigReader interface only defines a simple GetValue method, so there is no way to check for the presence of a configuration subsection.

More concretely, imagine I were trying to parse any configuration into the following type.

type Nested = { Key: string }

type Section = { SubSection: Nested option }

At some point, the generic version of parseFSharpOption would need to recurse into parseInternal to build an object of type Nested. This should only happen when there are some values prefixed by "Section" in the input configuration. I had troubles implementing exactly this: GetValue does not return a result for "non-terminal" keys, therefore there is no way of knowing if we should be trying to build the nested object at all.

This could probably be fixed by adding a GetSection : string -> IConfigReader option method to that interface. Do you have any other ideas of how to tackle this? Would you accept such a fix or do you see any problems with it?

aurecchia avatar Jul 30 '21 22:07 aurecchia

Hi @aurecchia, unfortunately I am not very familiar with the internals of the library. I am literally just a maintainer here accepting smaller PRs and updating dependencies every now and then.

queil avatar Aug 14 '21 09:08 queil

Sorry for the delayed reply, @aurecchia. Due to personal reasons, I am not actively working on any Open source stuff.

Regarding the solution you are proposing, I feel the better way would be to use the parseFSharpRecord function.

Here is a high-level approach that I can think of.

  1. Move the parseFSharpOption to be a part of the parseInternal function declaration like how we declared parseFSharpRecord function
  2. Then, in the parseFSharpOption function, like how we are treating the string types(null and empty checks), have a check for the FSharpRecord type like we do in the parseInternal function and call the parseFSharpRecord function with the appropriate parameters.

I am open to reviewing your changes once you are done. As long as it is not introducing any breaking changes, I am good :)

Thanks, @queil, for pitching in. We would need your help in publishing this once it is ready!

tamizhvendan avatar Aug 24 '21 12:08 tamizhvendan

Hi @tamizhvendan, don't worry about the delayed reply. I was playing around with a different approach than the one you suggested because I wanted parseFSharpOption to be able to parse any 'T. I was a bit destructive in the experiment I did because I was just trying to figure out the internals of the library. If I can manage to find a bit of time, I will try to fix this and submit a patch.

aurecchia avatar Aug 24 '21 17:08 aurecchia