Changes to the Types, and therefor the public API

[cies]

The new Node data structure as by @HuwCampbell's PR looks like this:

data Node = VTable    Table
          | VITable   Table
          | VTArray   [Table]
          | VString   Text
          | VInteger  Int64
          | VFloat    Double
          | VBoolean  Bool
          | VDatetime UTCTime
          | VArray    [Node]

While the old data struct that I came up with, is actually a combination of two data types, and looks like:

data Node = NTValue TValue
          | NTable  Table
          | NTArray [Table]
-- [...]
data TValue = VString   Text
            | VInteger  Int64
            | VFloat    Double
            | VBoolean  Bool
            | VDatetime UTCTime
            | VArray    [TValue]

Now my main questions regarding the new data structure are:

• What is a VITable, how is it different from a VTable, and why do we need it?
• The VArray can now contain anything of type Node; I guess in my old data structure I tried to make sure that VArrays can only contain values (TValue). I do not know if missing this bit of type safety is a real issue (I guess not). And I also wonder why we still need a difference between VTArray and VArray; but imagining how had it will be for modify the code in that direction hints that it is probably better this way.

Please know that this is one of my first proper Haskell project, so any feedback is welcome.

If you (any reader, but @HuwCampbell in particular) have any ideas on what could be improved to get this lib to higher standard please let me know (I'd like to push for inclusion in Stackage after I got some more feedback).

Cheers!

[HuwCampbell]

Hi. I flattened out the structure because TOML 0.4 allows inline tables, which are tables defined as values. In general I think the improvements are pretty good, all tests pass, string handling is improved, ....

The VITable is probably the bit I am least happy about however, its purpose is to hold an implicitly created table which can be redefined. For example:

[A.B]
[A]
  d = 2

works correctly and a A can be defined without error. I actually didn't export the constructor for this type and flatten it away before returning the result of the parse. It's not a good programming technique though, as it's too easy to add really bad (crash) bugs if one isn't careful. I would be happy if the two commits adding it were backed out for a better solution if anyone can come up with one. Something like:

data Node = VTable Table Explicitness
...
data Explicitness = Explicit | Implicit

maybe? This adds its own issues for the consumer of the library though.

[cies]

Hmm. Indeed this is an interesting challenge. What about keeping the notion of implicitness in a separate structure? In that case "flattening" it is merely discarding the separate structure.

Another alternative would be to request a small change in the TOML definitions that simply says that "implicitness" is the same as "without any keys defined in it". I reckon this would make make it easier to implement parsers in general.

Thanks for chiming in!

[HuwCampbell]

I had a thought, although generally I dislike the State monad, it could be used to keep track of this information and not stuff around with the type constructors.

EDIT: yes, you're insight lines up with my implementation idea.

[cies]

@HuwCampbell Would you like to make a different PR? Because I'm tempted to pull your changes in and make that API the new public API (easy since we're still pre-1.0 and still not in Stackage).

[HuwCampbell]

It's over in #10

[cies]

@HuwCampbell Looks really good. I'm testing and merging it in. Thanks a lot for this -- it makes the library (and especially it's pubAPI) a lot nicer! Good this is done before 1.0 :)

[cies]

@HuwCampbell I just cleaned up your code a bit and fixed some things throughout the code. I hope you like it in it's current shape.

Now I want to get some CI set up for it, ask once more for a review, publish a "1.0" and request Stackage inclusion. @HuwCampbell anything I miss?