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

Offer Abstract Delegators #153

Open
amihaiemil opened this issue Jan 14, 2019 · 11 comments
Open

Offer Abstract Delegators #153

amihaiemil opened this issue Jan 14, 2019 · 11 comments

Comments

@amihaiemil
Copy link

amihaiemil commented Jan 14, 2019

The API should offer abstract implementations for both JsonObject and JsonArray. These implementations will encapsluate a JsonObject and JsonArray delegator respectively , in order to leverage polymorphism.

E.g.

public abstract class AbstractJsonObject implements JsonObject {
    private final JsonObject delegate;

    public AbstractJsonObject (final JsonObject delegate) {
        this.delegate = delegate;
    }

    @Override
    public final JsonArray getJsonArray(final String name) {
        return this.delegate.getJsonArray(name);
    }
    //all other implementations, all delegated 
}

Here are some examples of usage (object orientation):

Polymorphic entity: https://github.com/amihaiemil/docker-java-api/blob/master/src/main/java/com/amihaiemil/docker/RtImage.java (extends JsonResource which is the same concept).

2 Articles:

https://www.amihaiemil.com/2017/10/16/javaee8-jsoncollectors-oop-alternative.html
https://www.amihaiemil.com/2017/06/14/non-flushable-jsonobjectbuilder.html (this is for JsonObjectBuilder)

This should be an API addition, obviously, since these classes are provider-agnostic, they are abstract and do not care about any concrete implementation.

If this is wanted, I'll make a PR myself.

@amihaiemil
Copy link
Author

@m0mus This Issue is almost 1 year old. Any thoughts on it? :D

@m0mus
Copy link
Contributor

m0mus commented Nov 25, 2019

@amihaiemil Isn't it the same as adding add(List<JsonValue> values) method to JsonArrayBuilder?

@amihaiemil
Copy link
Author

amihaiemil commented Nov 25, 2019

@m0mus No. With this feature, I'd like to be able to do the following:

public final class Car extends AbstractJsonObject {
    public Car(final JsonObject json) {
        super(json);
    }

   // some accessor methods to read attributes from the underlying json.
}

So Car is also an instance of JsonObject. And AbstractJsonObject would come from the API offering delegator methods (so we don't have to reimplement them in every implementation of JsonObject).

Same for JsonArray.

Makes sense?

@amihaiemil
Copy link
Author

amihaiemil commented Nov 25, 2019

@m0mus See my JsonResource class, which should come from the JSON-P api. I shouldn't have to implement it everytime.

@amihaiemil
Copy link
Author

amihaiemil commented Nov 26, 2019

@m0mus I would like to make a PR with the class JsonResource and the analogue one for JsonArray.

I would like to name them AbstractJsonObject and AbstractJsonArray respectively. Again, they will look as follows:

/**
 * Abstract JsonObject. Extend this class if you want to implement your own
 * JsonObject, so you don't have to re-implement all of its methods.
 */
public abstract class AbstractJsonObject implements JsonObject {
    private final JsonObject delegate;

    public AbstractJsonObject(final JsonObject delegate) {
        this.delegate = delegate;
    }

    //here, implement all of JsonObject's methods, delegating the calls to this.delegate
}

and

/**
 * Abstract JsonArray. Extend this class if you want to implement your own
 * JsonArray, so you don't have to re-implement all of its methods.
 */
public abstract class AbstractJsonArray implements JsonArray {
    private final JsonArray delegate;

    public AbstractJsonArray(final JsonArray delegate) {
        this.delegate = delegate;
    }

    //here, implement all of JsonArray's methods, delegating the calls to this.delegate
}

Will you accept it? I want to know before going through all the Contribution paperwork.

@amihaiemil amihaiemil changed the title Offer Abstract Delegators/Envelopes Offer Abstract Delegators Jan 13, 2020
@amihaiemil
Copy link
Author

@m0mus Can you have a look here again pls?

I really use this pattern a lot in my projects and I think these classes should be part of the API, since they are provider-agnostic.

@m-reza-rahman
Copy link

Now that things are moving again, how do we determine how to resolve proposed contributions like these? Some guidance would be highly appreciated. Looks like this has been sitting unresolved for a while now.

Reza Rahman
Jakarta EE Ambassador, Author, Blogger, Speaker

Please note views expressed here are my own as an individual community member and do not reflect the views of my employer.

@m0mus
Copy link
Contributor

m0mus commented Feb 1, 2020

@m-reza-rahman I asked for PR some time ago. When it's done we will review it and make decision will we merge it or not. @amihaiemil asking will I accept it or not doesn't make sense. I need to see it, make sure that tests are passing, etc.

@m0mus
Copy link
Contributor

m0mus commented Feb 1, 2020

I am skeptical about this idea anyway. It doesn't fit into the API design. In JSONP data structures are defined in interfaces and concrete classes are constructed using Json class or using factories.

@m-reza-rahman
Copy link

To be honest I have looked at this closely and I too am not sure what to think. It seems like an interesting idea but probably not an enhancement that is very critical. It would help if I knew other proven JSON processing APIs have similar capabilities. What do the other contributors and committers of this project think? Are they paying any attention to this discussion?

Either way I agree the first step is actually submitting a working PR, and maybe starting a discussion on the project alias.

Reza Rahman
Jakarta EE Ambassador, Author, Blogger, Speaker

Please note views expressed here are my own as an individual community member and do not reflect the views of my employer.

@amihaiemil
Copy link
Author

@m0mus @m-reza-rahman I've made a PR. Let's move the conversation there :)

amihaiemil added a commit to amihaiemil/jsonp that referenced this issue Feb 2, 2020
amihaiemil added a commit to amihaiemil/jsonp that referenced this issue Feb 2, 2020
amihaiemil added a commit to amihaiemil/jsonp that referenced this issue Feb 2, 2020
Signed-off-by: amihaiemil <[email protected]>
amihaiemil added a commit to amihaiemil/jsonp that referenced this issue Feb 2, 2020
Signed-off-by: amihaiemil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants