sdk/java: add CoreConfig class #1055
Conversation
|
PTAL @boymanjor @erykwalder |
|
|
||
| public static class Builder { | ||
| @SerializedName("is_generator") | ||
| private Boolean isGenerator; |
There was a problem hiding this comment.
This value will default to null. I'm assuming we actually want this to default to false, in which case we should use boolean. In general, its better to use boolean (unless handling null values is required).
There was a problem hiding this comment.
Hmm, very interesting! TBH I typed Boolean with the capital B more or less by accident, but now that you have me thinking, I suspect that capital-B Boolean may in fact be a bit better in this case.
I'm assuming we actually want this to default to false
I suspect this is not accurate. There's no particular reason false is better than true when there is no value returned from the server. Defaults are okay as a convenience mechanism for input, but they seem irrelevant when we're talking about output.
What null would imply here is "undefined", "no information", or even "there's a bug in the response-generating code and you should not rely on this value". I would actually love to be able to mark some (but not all) of these fields as "required", so that the JSON deserializer would throw an exception if the serialized version doesn't contain certain fields. Unfortunately, I dont think GSON will be adding that any time soon.
FWIW, what will happen here if you try to use a null Boolean object is a null pointer exception, which I think is ultimately better than arbitrarily assuming falsiness. Admittedly it's the lesser of two evils.
There was a problem hiding this comment.
I see your point, but this is on the Builder. Really we're talking about what should happen if you don't call setIsGenerator before calling configure. Leaving it as Boolean makes setIsGenerator a required call. That's fine, just wanted to be sure that is what we wanted.
There was a problem hiding this comment.
@boymanjor Ah, thanks for the clarification, I thought you were referring to the Boolean everywhere, which again I kinda just used by accident. I fully agree with using lower-case boolean here. Thanks!
There was a problem hiding this comment.
+1 to providing reasonable defaults in the Builder.
Also, more generally, it looks like we use int and long, not Integer and Long. I'd vote for consistency and use boolean including across response objects.
There was a problem hiding this comment.
Yeah I think the consistency thing is compelling. I wasn't sure how to weigh that until you pointed out that we also have ints/longs in this exact object. So, why pretend to solve the invalid-response problem just for bools? Until we have a proper answer for doing client-side validation, I should just quit worrying about it. Lower-case 4 lyfe.
| public Date configuredAt; | ||
|
|
||
| @SerializedName("is_signer") | ||
| public Boolean isSigner; |
There was a problem hiding this comment.
Generally, it looks like we use int and long in the SDK, not Integer and Long. I'd vote for consistency and use primitives across all objects.
There was a problem hiding this comment.
Plus, it's a little easier on the SDK user. The object types are easy to use incorrectly. Autoboxing can cause some really unexpected behavior, like a NullPointerException here:
if (config.isSigner == true) {
// ...
}
| @SerializedName("build_config") | ||
| public BuildConfig buildConfig; | ||
|
|
||
| public Map<String, Object> health; |
There was a problem hiding this comment.
is there any more schema to this that we can provide? it's a little unclear how to use this as-is. If I understand the cored code right, this map would look something like:
{
"errors": {
"generator": "error generating new block"
}
}
Maybe instead of a Map<String,Object> this should be a separate class? Even if we only expose a method that returns this map, it'll give us the flexibility to introduce more convenience methods down the road.
There was a problem hiding this comment.
I think this also might be the most useful of the fields exposed here, so I don't feel like it's over-engineering to ensure we can make it easy to access.
There was a problem hiding this comment.
Agreed. Let's land this PR and follow up with something. How confident are you in the stability of the structure of the health object?
There was a problem hiding this comment.
Might want to ask @kr, I'm not really sure.
The CoreConfig class finally gives the Java SDK access to the config package's RPC calls, including: - /configure - /reset - /info
The CoreConfig class finally gives the Java SDK access to
the config package's RPC calls, including: