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

Sanity check for JSON::Builder.field(name, &block) #5218

Open
straight-shoota opened this issue Oct 31, 2017 · 3 comments · May be fixed by #10563
Open

Sanity check for JSON::Builder.field(name, &block) #5218

straight-shoota opened this issue Oct 31, 2017 · 3 comments · May be fixed by #10563

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Oct 31, 2017

JSON::Builder.field with a block argument expects that in the block argument exactly one value is appended to the builder. This can obviously not be checked at compile time, but it should be possible to add a runtime check.

It is currently difficult to catch an error because this code:

builder.object do
  builder.field "foo" { builder.string("bar") }
  builder.field "baz", "bar"
end

is effectively the same as

builder.object do
  builder.string "foo"
  builder.string "bar"
  builder.string "baz"
  builder.string "bar"
end

If for some reason, the block does not append a value, the object creation gets messed up and fails with Missing object value (or Expected string for object name) when a non-string value is involved. This is hard to debug, because an error will raise somewhere later at a point where there is not really the issue.

I think it would be possible to raise an error if state.name is true at the end of field(name, &block), though I'd like to get some feedback on that idea.

@straight-shoota
Copy link
Member Author

I'd also suggest to add #field?(name, &block) which does not raise but inserts nil.
This would simplify:

builder.field "foo" { bar.try(&.to_json_special(builder) || builder.scalar(nil) }
# to
builder.field? "foo" { bar.try(&.to_json_special(builder) }

@jhass
Copy link
Member

jhass commented May 29, 2019

Not sure about field?, is there anything wrong with having that as the default behaviour? I mean for the absent value case it's still an easy to spot mistake given you got a null where you didn't expect one.

But in general either solution seems fine to me :)

@straight-shoota
Copy link
Member Author

Yeah, that's sounds like a good way to simplify the API. Skipping the property entirely would also be an option, but harder to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants