-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add TOML support #57
base: master
Are you sure you want to change the base?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite nice. I've made a few comments.
public TomlProcessor() { | ||
} | ||
|
||
@NotNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this empty constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reported to the handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it is because of the try / catch. But it's a little bit convoluted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.