Add TOML support - #57
Conversation
Addresses vert-x3#49 Using toml4j adds TOML support. Note: I have used Jetbrains annotations in this commit, if that is not desired I can remove them and stick with standard Java 8 methods of ensuring method contracts when it comes to null values.
cescoffier
left a comment
cescoffier
left a comment
There was a problem hiding this comment.
Looks quite nice. I've made a few comments.
| public TomlProcessor() { | ||
| } | ||
|
|
||
| @NotNull |
There was a problem hiding this comment.
Do we really need these annotations?
There was a problem hiding this comment.
Thanks for the comment Clement.
Probably not is the answer. I tend to use them in my projects to clearly define whether parameters are nullable or not etc. But I can see them adding a bit of noise to the method signatures for little gain here, not to mention adding another dependency to the project.
I was actually drafting a commit to remove them so will push that.
| */ | ||
| public final class TomlProcessor implements ConfigProcessor { | ||
|
|
||
| public TomlProcessor() { |
There was a problem hiding this comment.
Why do we need this empty constructor?
There was a problem hiding this comment.
I actually added this to be explicit in the fact that nothing was done in the constructor. I can remove it and we can rely on the default constructor created implicitly.
| ) { | ||
|
|
||
| Toml toml = new Toml().read(inputStream); | ||
| Map<String, Object> asMap = Objects.requireNonNull( |
There was a problem hiding this comment.
This should be reported to the handler.
There was a problem hiding this comment.
Right now it is because of the try / catch. But it's a little bit convoluted
There was a problem hiding this comment.
This was actually a bit of defensive programming on my part as the toml4j library provides the toMap method without any documentation. For reference:
public Map<String, Object> toMap() {
HashMap<String, Object> valuesCopy = new HashMap<String, Object>(values);
if (defaults != null) {
for (Map.Entry<String, Object> entry : defaults.values.entrySet()) {
if (!valuesCopy.containsKey(entry.getKey())) {
valuesCopy.put(entry.getKey(), entry.getValue());
}
}
}
return valuesCopy;
}In the processor would you have preferred something more like:
Map<String, Object> asMap = toml.toMap();
if (asMap != null) {
JsonObject asJson = new JsonObject(asMap);
future.complete(asJson);
} else {
future.fail(
new NullPointerException("toml library returned a null map")
);
}Instead of relying on the catch?
|
Also, did you sign the Eclipse CLA? |
|
I have not signed the CLA as of yet, I will have a read and sign if I agree to it. |
Change requested for vert-x3#49 by Clement Escoffier Makes sense as do not gain a whole lot by having them present and they add another dependency to the project.
Change requested for vert-x3#49 by Clement Escoffier
Mention PR at: vert-x3/vertx-config#57
2 participants
Addresses #49
Uses toml4j to add TOML support for the version 0.4.0 of TOML.
At original time of writing the specification for TOML 0.5.0 was not released.
There are few potential libraries that can be used that support 0.5.0, this is one such example that uses ANTLR.
Other libraries may also be better suited than the initial one chosen, that is up for debate.
Note: I have used Jetbrains annotations in this commit, if that is not
desired I can remove them and stick with standard Java 8 methods of
ensuring method contracts when it comes to null values.