Implementing YamlMapDump class #40
Conversation
1 similar comment
1 similar comment
|
@amihaiemil Please check the commit, I'm not figuring out what is the problem with the coveralls check? |
There was a problem hiding this comment.
@SherifWaly Looks good! Just 2 minor comments. Also, please change the puzzle, so the ticket gets closed (change the parent issue number and adapt the text: "Needs 1 more implementor...")
| } | ||
|
|
||
| @Override | ||
| YamlMapping represent() { |
There was a problem hiding this comment.
@SherifWaly this should be public :D
| if (super.leafProperty(key) && super.leafProperty(value)) { | ||
| mapBuilder = mapBuilder.add(key.toString(), value.toString()); | ||
| } else if (super.leafProperty(key)) { | ||
| mapBuilder = mapBuilder.add(key.toString(), |
There was a problem hiding this comment.
@SherifWaly Alignment here looks funny :D. Can you move key.toString() on the next line? Like this:
mapBuilder = mapBuilder.add(
key.toString(), new YamlObjectDump(value)
);
or if the param line is longer than 80 chars, like this
mapBuilder = mapBuilder.add(
key.toString(),
new YamlObjectDump(value)
);
Same on the next 2
|
@SherifWaly the coveralls failed because the code coverage lowered. The class needs to be unit tested. You can do it, or add a puzzle for it if you already spent too much time on this :D |
1 similar comment
|
@amihaiemil Please check the last commit. I have a problem with checking the YamlNode in the unit test so I left a puzzle for that :D |
1 similar comment
|
@amihaiemil I have completed the test and removed the puzzle. Please check the code :) |
| * Collection<Object>). The return type or method ``represent()`` should be | ||
| * overridden with the proper subtype (e.g. YamlMapDump will have the | ||
| * method ``YamlMapping represent() {...}`` | ||
| * @todo #34:30m/DEV This interface should have 1 more implementor class: |
There was a problem hiding this comment.
@SherifWaly The parent ticket here is 38 :D it also has to be updated so the bot sees it as a new puzzle.
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright (c) 2016-2017, Mihai Emil Andronache | |||
| a2el * Copyright (c) 2016-2017, Mihai Emil Andronache | |||
There was a problem hiding this comment.
@SherifWaly Can you remove this "a2el"? :P
There was a problem hiding this comment.
I don't know how it was written in the code :D
| * @todo #38:30m/DEV Complete unit test for YamlMapDump class. | ||
| */ | ||
| @Test | ||
| public void representsSimpleDojo() { |
There was a problem hiding this comment.
@SherifWaly the test name is not right. And can you also add a puzzle for writing more unit tests? I would like to write more test cases later :)
1 similar comment
|
@amihaiemil Please check the last commit :) |
|
@SherifWaly nice, thanks :D |
|
@rultor merge pls |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 1min) |
This PR is for #38