-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix Issue #497 and add a testcase #699
Conversation
…ck empty jsonObject.
@@ -148,12 +148,16 @@ public boolean isMap(Object obj) { | |||
JSONObject jsonObject = toJsonObject(obj); |
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.
You could probably simplify this method with streams (Sorry for any syntax errors. I'm writing this in the browser):
JSONObject jsonObject = toJsonObject(obj);
try {
if(Objects.isNull(jsonObject.names()) return new ArrayList<>();
return jsonObject.names().toList().stream()
.map(Object::toString)
.collect(Collectors.toList());
} catch(JSONException e) {
// Rest of the code
}
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, thanks! I have seen all the suggestions you mentioned, and I will change it as soon as possible.
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 find that the return type of jsonObject.names()
is JsonArray
, which doesn't have method like toList
. But the jsonObject
has another method keySet
which return the set of key that is needed.
(PS: the implementation of jsonObject.names()
is that it iteratively adds the value in keySet
into JsonArray
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.
https://stleary.github.io/JSON-java/org/json/JSONObject.html#names--
https://stleary.github.io/JSON-java/org/json/JSONArray.html#toList--
JSONObject::names should return a JSONArray
JSONArray should have a method called toList() according to their documentation.
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.
Also, I updated the code snippet. Instead of casting (String) name you can do Object::toString
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 find that the version of org.Json in JsonPath is 20140107, where JSONArray
has no method toList
.toList
is introduced in version 20160807
. What could I do?
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.
Please check my latest two commits, thanks!
The code of method .name()
is as follows, I think it is not necessary to use .name()
. Just using .keySet()
will be fine.
public Iterator keys() {
return this.keySet().iterator();
}
public JSONArray names() {
JSONArray ja = new JSONArray();
Iterator keys = this.keys();
while (keys.hasNext()) {
ja.put(keys.next());
}
return ja.length() == 0 ? null : ja;
}
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.
Looks good
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.
It is my first time to contribute to the open source community, sincerely thanks for your suggestions and approval !
@@ -76,4 +78,23 @@ public void read_book_length() { | |||
assertThat(result).isEqualTo(4); | |||
} | |||
|
|||
@Test | |||
public void Issue497() { |
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.
Please change method names to something more descriptive. I don't want to have to go to github to find out what this issue is.
} | ||
|
||
@Test | ||
public void Issue497_2() { |
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.
Same here bud.
…something more descriptive
The method getPropertyKeys() in class JsonOrgJsonProvider doesn't check empty jsonObject. #497