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

JsonArray now can return the List of JsonElement #1209

Closed
wants to merge 2 commits into from

Conversation

ayarullin
Copy link

I think it may be very convenient: for example it will be very helpful, if we need to sort this JsonArray with custom comparator (Using Collections.sort())

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@ayarullin
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor

@NightlyNexus NightlyNexus left a comment

Choose a reason for hiding this comment

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

By the way, you can use the Iterator in the existing Gson releases.
That said, it seems like an asList does belong here, since this already implements Iterable.

*
* @return this array as List of JsonElement
*/
public List<JsonElement> getAsList() {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about asList()?

/**
* Returns this array as List of JsonElement
*
* @return this array as List of JsonElement
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant doc.

* @return this array as List of JsonElement
*/
public List<JsonElement> getAsList() {
return elements;
Copy link
Contributor

Choose a reason for hiding this comment

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

return Collections.unmodifiableList(new ArrayList<>(elements));

Copy link
Author

@ayarullin ayarullin Dec 20, 2017

Choose a reason for hiding this comment

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

Hm... Why? The idea of this metod is modify JsonArray by modifying List (for example for sorting).

That said, it seems like an asList does belong here, since this already implements Iterable.

We cannot convert Iterable to List without going over all elements. This is not optimal way.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i misunderstood the use case.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot
Copy link

CLAs look good, thanks!

@ayarullin
Copy link
Author

Hey guys?
I commited fixes, is it ok?

@lyubomyr-shaydariv
Copy link
Contributor

lyubomyr-shaydariv commented Apr 12, 2018

What if JsonArray could implement List<JsonElement> rather than just being an iterable?

@lyubomyr-shaydariv
Copy link
Contributor

By the way, there's 3b7bc45 at the JsonArrayImplementsList branch but it's not merged to master for some reason. I think that it breaks JsonArray.add return type, but in general it also looks like unfinished work. For example, JsonObject might implement Map<String, JsonElement> as well?

@ayarullin
Copy link
Author

@lyubomyr-shaydariv
No, this is because is breaks binary compatibility - look #683
By the way, JsonObject has entrySet method which returns the set of elements in the object.
Maybe returning the Map<String, JsonElement> may be redundant.
What do you think @NightlyNexus

@lyubomyr-shaydariv
Copy link
Contributor

@ayarullin Oh, I see. Thanks.

@lyubomyr-shaydariv lyubomyr-shaydariv mentioned this pull request Nov 15, 2019
@Marcono1234 Marcono1234 mentioned this pull request Aug 22, 2020
@Marcono1234
Copy link
Collaborator

Note that directly exposing the underlying ArrayList allows inserting Java null values which should not be possible (see also #1771). So maybe this pull request should either change the the list implementation of JsonArray.elements (which would always decrease performance), or better have asList() wrap the underlying list in a list which prevents null values.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Oct 16, 2022

This has been superseded by #2225. Thank you nonetheless for your effort on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants