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

Generic wrapped object #13

Closed
wants to merge 6 commits into from
Closed

Conversation

andreoss
Copy link

@andreoss andreoss commented Jun 8, 2020

Closes #3

@andreoss andreoss marked this pull request as ready for review June 8, 2020 00:37
Copy link
Owner

@victornoel victornoel left a comment

Choose a reason for hiding this comment

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

@andreoss thanks for the contribution, I've some comments for you, but this looks great.

I wonder what is your use case for this? I had the idea about it in #3 but I had no real use case in mind, so I would be very interested about knowing it if there is :)

.editorconfig Outdated Show resolved Hide resolved
@andreoss
Copy link
Author

andreoss commented Jun 8, 2020

I wonder what is your use case for this? I had the idea about it in #3 but I had no real use case in mind, so I would be very interested about knowing it if there is :)

Self-referencing interface would need a generic envelope

interface Self<T extends Self<? extends T>> {}

@GenerateEnvelope(generic = true) will provide an envelope which could be nested like so

class Impl implements Self<Impl> {}
class Wrap1 extends SelfEnvelope<Impl, Self<Impl>> {
    public Wrap1(final Self<Impl> wrapped) {
        super(wrapped);
    }
}
class Wrap2 extends SelfEnvelope<Impl, Self<Impl>> {
    public Wrap2(final Self<Impl> wrapped) {
        super(wrapped);
    }
}
final Self<Impl> env4 = new Wrap1(new Wrap2(new Wrap1(new Impl())));

While non-generic will be bounded to Impl and will not accept itself.

@andreoss andreoss closed this Jun 9, 2020
@andreoss andreoss reopened this Jun 9, 2020
@victornoel
Copy link
Owner

@andreoss I have not forgotten about this PR :)

@victornoel
Copy link
Owner

@andreoss I'm not clear about the example you gave me above.

Why can't you just write:

class Impl implements Self<Impl> {}
class Wrap1<T extends Self<? extends T>> extends SelfEnvelope<T> {
    public Wrap1(final Self<T> wrapped) {
        super(wrapped);
    }
}
class Wrap2<T extends Self<? extends T>> extends SelfEnvelope<T> {
    public Wrap2(final Self<T> wrapped) {
        super(wrapped);
    }
}
final Self<Impl> env4 = new Wrap1(new Wrap2(new Wrap1(new Impl())));

It works perfectly well.

By the way, I believe this kind of pattern is more usually wrote like this (without the ? extends):

interface Self<T extends Self<T>> {}

Copy link
Owner

@victornoel victornoel left a comment

Choose a reason for hiding this comment

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

@andreoss the main problem I see with this PR is the way we generate the new type parameter. We can't use any one and I find the code you propose a bit complex. I wonder if there are no available tooling in google auto common (that we already use here) or javapoet that can do that for us...

@victornoel
Copy link
Owner

I have created an issue at javapoet asking about it: square/javapoet#793

@victornoel
Copy link
Owner

@andreoss could you try using NameAllocator (see square/javapoet#793) instead of using some adhoc method to generate a fresh name? I think W, as you proposed, is a correct default name.

Copy link
Owner

@victornoel victornoel left a comment

Choose a reason for hiding this comment

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

@andreoss one small change for good style :) thank a lot, this looks very nice!

@andreoss andreoss requested a review from victornoel June 28, 2020 18:56
victornoel added a commit that referenced this pull request Mar 22, 2021
@andreoss andreoss closed this Nov 28, 2022
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

Successfully merging this pull request may close these issues.

Generic wrapped object
2 participants