Created module with converter and configuration source for YAML #35
Conversation
sobolewskikamil
force-pushed
the
yaml-module
branch
from
6307256
to
8bbd509
Compare
There was a problem hiding this comment.
Overall the changes looks good but some improvements are necessary.
JavaDoc should be provided, user's guide updated. I'm not sure about performance implications. Could you check if creating snake yaml serializer in the converter every time is fast enough or some pool of objects is necessary.
| import java.util.Iterator; | ||
| import java.util.Map; | ||
|
|
||
| public class MapIterable implements Iterable<ConfigurationEntry> { |
There was a problem hiding this comment.
Could you provide javadoc for this class?
|
|
||
| @Override | ||
| public Iterator<ConfigurationEntry> iterator() { | ||
| Iterator<Map.Entry<String, String>> iterator = map.entrySet().iterator(); |
There was a problem hiding this comment.
I think statically importing Map.Entry will make a code a bit more readable.
| private final Map<String, String> map; | ||
|
|
||
| public MapIterable(Map<String, String> map) { | ||
| this.map = map; |
There was a problem hiding this comment.
Please add Objects.requireNonNull() to make sure the object is not created with null map.
| import static java.lang.annotation.ElementType.METHOD; | ||
| import static java.lang.annotation.RetentionPolicy.RUNTIME; | ||
|
|
||
| // todo javadoc |
There was a problem hiding this comment.
The documentation is required for this annotation and it should show example usage.
| import static java.util.Collections.singletonMap; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class YamlConfigurationSource implements IterableConfigurationSource { |
There was a problem hiding this comment.
Could you provide javadoc for this class?
| requireNonNull(inputStream, "inputStream cannot be null"); | ||
|
|
||
| Reader reader = new UnicodeReader(inputStream); | ||
| this.properties = createProperties(reader); |
There was a problem hiding this comment.
This line should be inside try to make sure input stream is always closed properly.
| Reader reader = new UnicodeReader(inputStream); | ||
| this.properties = createProperties(reader); | ||
| try { | ||
| reader.close(); |
There was a problem hiding this comment.
Are you sure both reader and stream must be closed?
|
|
sobolewskikamil
force-pushed
the
yaml-module
branch
from
771a1c3
to
9904aab
Compare
No description provided.