Skip to content
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

Jump to bottom

json patches introduced to the OpenJDK copy in JDK-8246435 #636

Closed
wants to merge 1 commit into from
+44 −18
Closed

json patches introduced to the OpenJDK copy in JDK-8246435 #636

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 json/src/main/java/org/openjdk/skara/json/JSONArray.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -39,6 +39,24 @@ public JSONArray(JSONValue[] array) {
}
}

private void append(JSONValue value) {
Copy link
Member

@edvbld edvbld Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this to concat and explicitly take a JSONArray instead as parameter? I'm fine with having append, but append should be just append (that is, append a single item to the array).

Copy link
Member Author

@magicus magicus Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me there's no deeper semantic difference between "append" and "concat". To be precise, this method do a flattening append/concat, and should probably be named something like addAndFlatten to be completely unambiguous. But that kind of naming did not seem to fit in with the rest of the design here.

If you insist that "concat" means "flatten while adding" but "append" does not, sure, I can rename it.

Copy link
Member Author

@magicus magicus Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, now I see what you mean. You want a concat(JSONArray a) instead. Well, it might be good to have, but that does not really solve my problem, does it? :-)

Copy link
Member

@edvbld edvbld Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you have append(JSONValue v) and concat(JSONArray a) then code using such an API can easily have the code:

void appendOrConcat(JSONArray a, JSONValue v) {
    if (v.isArray()) {
        a.concat(v.asArray());
    } else {
        a.append(v);
    }
}

I'm fine with having appendOrConcat in JSONArray as well, but then I want it named appendOrConcat, not append. I agree that append is shorter, but it is too misleading of a name IMHO for a method that really does appendOrConcat.

Copy link
Member Author

@magicus magicus Jun 4, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get it. Your example code would not recursively inline arrays; it will only work for a single nested layer. I can't see how that would be helpful to anything but the most specific situations. What is it you want to achieve?

if (value instanceof JSONArray) {
for (var v : value.asArray()) {
append(v);
}
} else {
this.values.add(value);
}
}

public JSONArray(JSONValue value, JSONValue... values) {
Copy link
Member

@edvbld edvbld Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment about concat vs append, I would prefer that the constructor does append and not concat. One could imagine a static version of concat as well, e.g. public static JSONArray concat(JSONArray... arrays).

Copy link
Member Author

@magicus magicus Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care about a non-flattening constructor. You can add one if you need one. But I can change it to a static factory method instead, if you want. It's probably clearer, given that we can agree on a suitable name. (I'll accept "concat" if you insist, but I'm not persuaded that it's really better...)

this.values = new ArrayList<JSONValue>(values.length + 1);
append(value);
for (var v : values) {
append(v);
}
}

public JSONArray(List<JSONValue> values) {
this.values = new ArrayList<JSONValue>(values);
}
4 json/src/main/java/org/openjdk/skara/json/JSONBoolean.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@
package org.openjdk.skara.json;

public class JSONBoolean implements JSONValue {
boolean value;
private boolean value;

public JSONBoolean(boolean value) {
this.value = value;
4 json/src/main/java/org/openjdk/skara/json/JSONDecimal.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@
package org.openjdk.skara.json;

public class JSONDecimal implements JSONValue {
double value;
private double value;

public JSONDecimal(double value) {
this.value = value;
8 json/src/main/java/org/openjdk/skara/json/JSONNumber.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -22,11 +22,11 @@
*/
package org.openjdk.skara.json;

class JSONNumber implements JSONValue {
long value;
public class JSONNumber implements JSONValue {
private long value;

public JSONNumber(int value) {
this.value = (long) value;
this.value = value;
}

public JSONNumber(long value) {
10 json/src/main/java/org/openjdk/skara/json/JSONObject.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -109,6 +109,10 @@ public JSONObject putNull(String k) {
return this;
}

public JSONValue remove(String k) {
return value.remove(k);
}

public JSONValue get(String k) {
return value.get(k);
}
@@ -128,6 +132,10 @@ public boolean contains(String field) {
return value.containsKey(field);
}

public Set<String> keys() {
return value.keySet();
}

@Override
public String toString() {
var builder = new StringBuilder();
6 json/src/main/java/org/openjdk/skara/json/JSONString.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -22,8 +22,8 @@
*/
package org.openjdk.skara.json;

class JSONString implements JSONValue {
String value;
public class JSONString implements JSONValue {
private String value;

public JSONString(String value) {
this.value = value;
10 json/src/main/java/org/openjdk/skara/json/JSONValue.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -90,12 +90,12 @@ default List<JSONObject.Field> fields() {
return asObject().fields();
}

default boolean contains(String field) {
return asObject().contains(field);
default boolean contains(String key) {
return asObject().contains(key);
}

default JSONValue get(String field) {
return asObject().get(field);
default JSONValue get(String key) {
return asObject().get(key);
}

default JSONValue getOrDefault(String field, JSONValue fallback) {