-
Notifications
You must be signed in to change notification settings - Fork 101
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
Make sure lint results follow phab api specs #191
Conversation
@sectioneight @kageiit |
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.
Regression test?
(Integer) json.get("line"), | ||
(Integer) json.get("char"), | ||
line == null ? null : (Integer) line, | ||
charPosition == null ? null : (Integer) charPosition, | ||
(String) json.get("description"))); |
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.
description is optional as well
String code = (String) json.get("code"); | ||
String severity = (String) json.get("severity"); | ||
String path = (String) json.get("path"); | ||
Integer line = (Integer) json.opt("line"); |
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.
This will NPE because you are trying to typecast null to Integer
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.
Still does not address my previous comments
assertEquals(results.code, "_code"); | ||
assertEquals(results.severity, "error"); | ||
assertEquals(results.path, "foobar.java"); | ||
assertTrue(results.line == null); |
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.
we should not even be sending these fields if they are null
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.
Wait I think this does what its supposed to. nvm :)
assertEquals(results.code, "_code"); | ||
assertEquals(results.severity, "error"); | ||
assertEquals(results.path, "foobar.java"); | ||
assertTrue(results.line == null); |
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.
Wait I think this does what its supposed to. nvm :)
String path = (String) json.get("path"); | ||
Integer line = (Integer) json.opt("line"); | ||
Integer charPosition = (Integer) json.opt("char"); | ||
String description = (String) json.get("description"); |
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.
opt here. description is optional according to spec
Phab API specs does not require these 2 params. By forcefully casting the object we get back from json, we are inherently enforcing requirement.