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

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

Closed
wants to merge 1 commit into from

Conversation

magicus
Copy link
Member

@magicus magicus commented Jun 3, 2020

This PR contains the proposed changes to the json system made in JDK-8246435. If the same changes are made in Skara, tthe two code bases are kept from diverging.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Download

$ git fetch https://git.openjdk.java.net/skara pull/636/head:pull/636
$ git checkout pull/636

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 3, 2020

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@magicus magicus changed the title Patches introduced to the OpenJDK copy in JDK-8246435. json patches introduced to the OpenJDK copy in JDK-8246435 Jun 3, 2020
@openjdk openjdk bot added the rfr label Jun 3, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 3, 2020

Webrevs

Copy link
Member

@edvbld edvbld left a comment

Hi Magnus,

thanks for contributing! Looks really nice in general, I just had two small comments about the changes to JSONArray.

Thanks!
Erik

@@ -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

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?

}
}

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...)

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 25, 2020

@magicus This pull request has been inactive for more than 3 weeks and will be automatically closed if another 3 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@magicus
Copy link
Member Author

@magicus magicus commented Jun 25, 2020

Yeah, yeah... Stop whining, bot.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 16, 2020

@magicus This pull request has been inactive for more than 3 weeks and will be automatically closed if another 3 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 6, 2020

@magicus This pull request has been inactive for more than 6 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

@bridgekeeper bridgekeeper bot closed this Aug 6, 2020
@magicus magicus deleted the ihse-json-patches branch Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants