Skip to content
This repository has been archived by the owner on Oct 19, 2020. It is now read-only.

Should TomlObjectFactory.Update() return this to be more "fluent?" #68

Closed
akeeton opened this issue Jan 11, 2019 · 3 comments
Closed

Should TomlObjectFactory.Update() return this to be more "fluent?" #68

akeeton opened this issue Jan 11, 2019 · 3 comments

Comments

@akeeton
Copy link

akeeton commented Jan 11, 2019

private static T Update<T>(TomlTable table, string key, T obj)
    where T : TomlObject
{
    if (table.TryGetValue(key, out var _))
    {
        table.SetRow(new TomlKey(key), obj);
        return obj;
    }
    else
    {
        throw new InvalidOperationException($"Cannot update because row with key '{key}' does not exist.");
    }
}

Why does TomlObjectFactory.Update() return obj instead of this? I'm looking to do something like this:

Toml.
    ReadString("x = 1").
    Update("x", 1).
    ToString()

Is there a use case for returning obj that I'm not aware of? Thanks.

@paiden
Copy link
Owner

paiden commented Jan 13, 2019

When I wrote that API I had more the use case of configuring the TomlObject metadata in mind.

But on a second thought I also think, that a fluent API would be nicer, as configuring the meta data for a TOML object (like comments) is a somewhat rare use case.

In commit 0ca6890 you can see a first conceptual version of an improved API that allows to do fluent configuration. Via the optional lambda also the metadata configuration can be done without a explicit fetch from the underlying table. The following code shows the different API usage.

Old:

tml.Update("x", 2).AddComment("New X Value");
tml.Update("y", 3).AddComment("New Y Value");

New:

tml
    .Update("x", 2, x => x.AddComment("New X Value"))
    .Update("y", 3, y => y.AddComment("New Y Value"))

while in reality I expect most usage scenarios too look like this:

tml
    .Add("z", "Completely New")
    .Update("x", 2)
    .Update("y", 3)

Currently I think, also add/create operations should follow the same concept.

@paiden
Copy link
Owner

paiden commented Jan 30, 2019

So while updating the test for the new API I found out that the fluent API still was not very usable.

I really had a had time fixing the tests and getting them correct. The problem is that both cases of returned objects could be expected depending on how the consumer code is written. So IMO it was essential that using the API states more explicitly on what someone is operating.

Another issue was that the already complex overload resolution got even worse with the new default Arguments.

A first version of the thing I came up with, to solve these issues, can be found in: 64283db
The examples from above would look like this with the new API:

tml.Update("x", 2).ConfigureAdded(x => x.AddComment("New X Value")
    .And.Update("y", 3).ConfigureAdded(x => x.AddComment("New Y Value");
tml .Update("x", 2)
    .And.Update("y", 3)

Here's a more complex example that IMO shows what the benefit of the new Result type object fluent design is:

var t = Toml.Create();
t.Add("SubTable",
    t.CreateEmptyAttachedTable()
        .Add("A", (System.Collections.Generic.IEnumerable<int>)(new int[] { 1, 2, 3 }))
            .ConfigureAdded(a => a.AddComment("This is the array"))
        .And.Add("AnotherSubTable", t.CreateEmptyAttachedTable())
            .ConfigureAdded(st => st.Add("InsideNewSubtable", "Yes I'am")
                .And.Add("WhatAboutMe", "Yes I'm also here"))
        .Owner)
    .And.Add("I", (long)1)
    .And.Add("F", 1.0);

that will produce

I = 1
F = 1.0

[SubTable]
#This is the array
A = [1, 2, 3]

[SubTable.AnotherSubTable]
InsideNewSubtable = "Yes I'am"
WhatAboutMe = "Yes I'm also here"

Hopefully so complex fluent API usage scenarios will not be that common.
But with new new API at least you can somewhat reason about, what will go on.
With the previous API's such a scenario was IMHO not understandable at all.

paiden added a commit that referenced this issue Feb 2, 2019
The new result object allows consumers of the API
to better distinguish on what is returned / accessed
on the consumer side.

Also returning both ojbects, the added and its container
allow the ocnsturction of complex object graphs i
a fluent way.

Related: #68
paiden added a commit that referenced this issue Feb 2, 2019
The new result object allows consumers of the API
to better distinguish on what is returned / accessed
on the consumer side.

Also returning both objects - the added and its container -
allows the construction of complex object graphs in
a fluent way.

Related: #68
paiden added a commit that referenced this issue Feb 3, 2019
The new result object allows consumers of the API
to better distinguish on what is returned / accessed
on the consumer side.

Also returning both objects - the added and its container -
allows the construction of complex object graphs in
a fluent way.

Related: #68
paiden added a commit that referenced this issue Feb 3, 2019
The new result object allows consumers of the API
to better distinguish on what is returned / accessed
on the consumer side.

Also returning both objects - the added and its container -
allows the construction of complex object graphs in
a fluent way.

Related: #68
paiden added a commit that referenced this issue Feb 3, 2019
The new result object allows consumers of the API
to better distinguish on what is returned / accessed
on the consumer side.

Also returning both objects - the added and its container -
allows the construction of complex object graphs in
a fluent way.

Related: #68
@paiden
Copy link
Owner

paiden commented Feb 18, 2019

Released with 0.11.0

@paiden paiden closed this as completed Feb 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants