Issue #15 WarFile.java:134-135: Implement validation of YAML inside. #28
Conversation
|
@tmjee Thanks for your pull request, let me find someone who can review it |
|
@dmzaytsev please review, thanks |
| try { | ||
| new Yaml().load(text); | ||
| } catch (final YAMLException exception) { | ||
| valid = false; |
There was a problem hiding this comment.
@tmjee does YAMLException provide extended information about issues in the yaml file?
if so it would better inform the user about details instead just say the file is valid or not.
what do you think?
There was a problem hiding this comment.
@dmzaytsev throw YAMLException up so message can be sticked into MojoExecution exception
|
@tmjee |
|
@tmjee please see 7 comments above |
|
@dmzaytsev Test for |
| @Test(expected = YAMLException.class) | ||
| public void testInvalidYaml() throws Exception { | ||
| // @checkstyle StringLiteralsConcatenationCheck (6 lines) | ||
| final String invalid = "Some illegal Prefix\n" |
There was a problem hiding this comment.
@tmjee String.format() or StringBuilder can be used here instead checkstyle suppression.
|
@tmjee thank you |
| try { | ||
| this.validYaml(text); | ||
| } catch (final YAMLException yamlException) { | ||
| final String msg = |
There was a problem hiding this comment.
|
@tmjee a few comments from me |
|
@yegor256 Done. |
| try { | ||
| this.validYaml(text); | ||
| } catch (final YAMLException exception) { | ||
| final String msg = |
There was a problem hiding this comment.
@tmjee this is still a redundant variable: http://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html
|
@tmjee a few more comments from me |
|
@yegor256 done. |
| */ | ||
| private String readFile(final ZipFile warf, final ZipEntry entry) | ||
| throws MojoFailureException { | ||
| InputStreamReader reader = null; |
There was a problem hiding this comment.
@tmjee the use of null tells us that the design is wrong, see http://www.yegor256.com/2014/05/13/why-null-is-bad.html After taking a closer look at this code I understood what is wrong. Your try/catch block is too big. This is how this code should look:
final Reader reader;
try {
reader = new InputStreamReader(warf.getInputStream(entry));
} catch () {
throw ...
}
try {
return CharStreams.toString(reader);
} finally {
reader.close();
}This is how it should be. Keep in mind that every time you see null - it's a bad design.
|
@tmjee one more comment from me. Keep in mind, |
|
@yegor256 done |
|
@rultor try to merge |
|
@dmzaytsev Much obliged! I have added 22 mins to your account in payment "66848066", 190 hours and 58 mins spent... review comments (c=7) added as a bonus... +22 added to your rating, current score is: +1340 |
|
@rultor deploy now pls |
Issue #15