-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Implemented NoCommentsYamlLine #97
Conversation
@amihaiemil Please check this commit :). I have made |
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.
@SherifWaly Looks good! :D Just a few comments
while(i < string.length() && string.charAt(i) != '"') { | ||
i++; | ||
} | ||
} else { |
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.
@SherifWaly For readability, I think you can remove this else
and just leave i++
, since it's going to be executed anyway, no matter the previous ifs
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.
@amihaiemil This extra else
had a test that it will fail at. I've removed it and changed the trimsCommentsAtEndOfLine()
test to check that it doesn't fail now.
|
||
@Override | ||
public String trimmed() { | ||
String string = this.line.trimmed(); |
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.
@SherifWaly Rename this variable to trimmed
maybe? string
is too generic :D
* NoCommentsYamlLine doesn't remove # escaped in a string. | ||
*/ | ||
@Test | ||
public void doesnotTrimsEscapedHash() { |
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.
The name here should be doesNotTrimEscapedHash()
(N and Trim instead of Trims)
} | ||
|
||
/** | ||
* NoCommentsYamlLine doesn't remove # escaped in a string. |
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.
" doesn't remove escaped #"
@amihaiemil Please check it now I've also made a small change at |
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.
@SherifWaly the else
change must be reverted -- sorry, my mistake :D
} | ||
i++; |
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.
@SherifWaly ah, sorry, the else
is needed, since you only want i++
executed if the flow hasn't gone through the 2 ifs.
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.
@amihaiemil I'm really confused but that is what I'm thinking at, there are 2 cases:
First: it's a "
char and the while loop will stop when it finds another one but i
will not be incremented so the modified test fails.
Second: it's any other char so i should be incremented.
What's your opinion :D ?
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.
@SherifWaly Well, I tried now the code, and the test without the else
and without the modification passes fine. How did it fail for you? :D (you said you modified it because it failed once you removed the else
).
After running the code, my opinion is that it's correct without the else
and also the original test passes.
It's correct without the else because the while
in else if
stops at a "
- then you need to make i++
to get to the next char :D
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.
@amihaiemil I wanted to say that the modified test fails if I used else
but the first one passes in both cases :D
So just I modified the test to make the code with else
fails :D
I think I should add both tests to make the difference clear.
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.
@SherifWaly I see now.
Yes, add both tests, but leave the else
out, this is how it's correct. The modified test fails with the else
because the else
introduces a bug, the i
misses an incrementation.
Makes sense now? Both tests are good and have to pass - the code has to satisfy both of them.
@@ -99,18 +99,18 @@ public void trimsCommentsLine() { | |||
@Test | |||
public void trimsCommentsAtEndOfLine() { | |||
YamlLine noComments = new NoCommentsYamlLine( | |||
new RtYamlLine(" this isn't comment #here is the comment", 1) | |||
new RtYamlLine(" \"this isn't comment\" #here is the comment", 1) |
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.
@SherifWaly make this test as it was :D
This test failed with the else
change, because it actually found a bug.
You changed the test to pass, instead of fixing the bug -- it's a common mistake, I also used to do it, not realizing that the code should be changed to pass the test, not the other way around (of course, assuming the test is well written, which this test was)
@SherifWaly Should be ok now, thanks :) |
@rultor merge please |
@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) |
@amihaiemil Sorry for the misunderstand that I've made by changing the test :) |
@SherifWaly No worries, it's good that we caught it :) This proves the tests help and coverage actually matters |
@amihaiemil Yes, testing is really important to catch such issues :) |
PR for #95