Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate unaryPlus for Json array builder in favor of add. #462

Closed
wants to merge 1 commit into from

Conversation

sandwwraith
Copy link
Member

Mainly because of #418

There are three ways for it:

  1. Do nothing.
  2. Deprecate unaryPlus only for Number (and probably other primitives)
  3. Deprecate all overloads, including one with JsonElement.

This PR implements 3), but I suggest considering 2) as a resolution, because using add everywhere seems verbose and overhead-ish to me (see diff of JsonArraySerializersTest). However, this will lead to inconsistency: JsonElement would be added via unaryPlus and other primitives via add. WDYT, @qwwdfsad ?

@sandwwraith sandwwraith requested a review from qwwdfsad May 21, 2019 09:14
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented May 22, 2019

This is a subtle topic. I agree that add is too verbose and I am fine with an option when both add and + are present for every element type except Number to leave some options for ones who prefer consistency over verbosity.

But I think we can do better. Let's take a look at the JsonArraySerializersTest.

return jsonArray {
    add(1)
    add(JsonNull)
    add(jsonArray {
        add("nested literal")
    })
    add(jsonArray { })
    add(json {
        "key" to "value"
    })
}

what do we want to ideally see here? A consistent picture without a boilerplate.
Say, something like this:

return jsonArray {
    jsonLiteral(1)
    jsonNull // <- I have doubts about this one
    jsonArray {
        JsonLiteral("nested literal")
    }
    jsonArray { } 
    jsonObject {
        "key" to "value"
    }
}

what do you think about that approach? I tend to think that + and add are really unnecessary here and the only choice we have to make is whether these builders should look like plain json elements JsonArray { ... } or like functions jsonArray { ... }

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(obligatory comment for GitHub to finish the review)

@sandwwraith
Copy link
Member Author

It looks nice but seems a bit overkill, since this DSL would not be used often (and when it is used, I assume, it would contain one-two calls in a for loop). There is also a problem which can cause a mistake you've already made in the example: jsonArray { jsonLiteral(1) } works, but jsonArray { JsonLiteral(1) } (notice capital letter) does not.

@qwwdfsad
Copy link
Collaborator

It looks nice but seems a bit overkill, since this DSL would not be used often

I am not sure this is a valid point. Either we should provide a best-effort DSL or nothing at all.

There is also a problem which can cause

Then probably it's worth to make builder extensions undistinguishable from constructors?

@sandwwraith
Copy link
Member Author

Closing in favor of #794

@qwwdfsad qwwdfsad deleted the issue_418 branch September 19, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants