-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fixed #1245 #1246
Fixed #1245 #1246
Conversation
LGTM but can you please add a test for the fix. |
Yes, but not right now. Good point!
|
Could you please add a test for it so we can merge this in 😄 ? We already closed the related issue. |
Done! Will merge when I see that the tests pass. |
@@ -494,4 +494,26 @@ public void testCompareToNotEqual() throws IOException { | |||
assertTrue(style1.compareTo(style2) > 0); | |||
assertFalse(style2.compareTo(style1) > 0); | |||
} | |||
|
|||
|
|||
public void test1245AndOxfordComma() throws URISyntaxException, IOException { |
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.
Dont call it 1245 please 😄 Name it by what it does check.
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.
Yeah, I do not really know what it checks. ;-) (But I'll figure something out.)
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.
Something with empty strings as values?! maybe the oxford comma is even a separate test case?
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 was considering a separate test case, but it would (could) look exactly the same, so I thought it made more sense to have both in one.
This should fix #1245 so that it is possible to set empty strings is jstyle-files as
key=""