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

Feature Request: Register Interfaces on ExternalSerialization#setup #12

Closed
jpe42 opened this issue Nov 13, 2016 · 16 comments
Closed

Feature Request: Register Interfaces on ExternalSerialization#setup #12

jpe42 opened this issue Nov 13, 2016 · 16 comments

Comments

@jpe42
Copy link
Contributor

jpe42 commented Nov 13, 2016

For @CompiledJson classes, would it be possible to register generated JsonReader/Writers with the interface they implement as well? The behavior I am observing is that only the annotated class is registered (DslJson#registerReader). Maybe adding a list of superclasses to the @CompiledJson annotation would be the easiest route?

Currently, I have an if statement for each expected interface and then manually pass the corresponding implementation class.

@zapov
Copy link
Member

zapov commented Nov 13, 2016

Writers are already looked up through their inheritance chain.
Readers are not since they go in separate direction.

I suppose you don't want to register it manually through: https://github.com/ngs-doo/dsl-json/blob/master/library/src/main/java/com/dslplatform/json/DslJson.java#L271 to avoid referencing generated code?
I guess baseReaders (or something like that) makes sense on CompiledJson.

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 13, 2016

Exactly, trying to avoid any maintenance or breakage from every time a new ExternalSerialization is generated.

@zapov
Copy link
Member

zapov commented Nov 13, 2016

I've added some initial implementation. Let me know if you find any issues, have any comments.

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 14, 2016

Works perfectly, thank you!!!

@jpe42 jpe42 closed this as completed Nov 14, 2016
@zapov
Copy link
Member

zapov commented Nov 14, 2016

btw. I realized that you can get the same behavior with something along the lines of

dsljson.registerReader(interface.class, dsljson.tryFindReader(implementation.class))

but tryFindReader is protected :(

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 14, 2016

I think I like that even better :), feels more flexible and avoids conflicts. That way you don't end up enforcing a reader/implementation for a given Interface for every DslJson that is constructed.

@jpe42 jpe42 reopened this Nov 14, 2016
@zapov
Copy link
Member

zapov commented Nov 14, 2016

So you are fine with me removing that baseReaders feature?

I guess I should make tryFindReader/tryFindWriter public :/

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 14, 2016

I'm happy with either or both. Why are you against making tryFind* public?

@zapov
Copy link
Member

zapov commented Nov 14, 2016

I'm not :)
protected is almost the same as public :D

I guess the reason I didn't make them public initially is because of dualism, since getObjectReader should be public then.
And people might get confused with that. I guess I'll leave that protected, but will make try public

@zapov
Copy link
Member

zapov commented Nov 16, 2016

tryFind methods are now public

@zapov zapov closed this as completed Nov 16, 2016
@jpe42
Copy link
Contributor Author

jpe42 commented Nov 17, 2016

Is there a public snapshot maven repository available to test out these changes?

@zapov
Copy link
Member

zapov commented Nov 17, 2016

For some reason I dont have a habit od -snapshot. If you make a pr with all the required pom changes I can release a snapshot version on Maven (never did so yet). If you just want to test it, you can subclass and test

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 17, 2016

Sorry, I can't be of any help there, I'm completely lost in the maven world, Gradle converted me a while ago. I have been able to confirm it works by simply building locally. Really just wanted to release a library that depends on that feature.

@zapov
Copy link
Member

zapov commented Nov 17, 2016

Why not subclass as temp solution?

class MyDslJson extends DslJson

@jpe42
Copy link
Contributor Author

jpe42 commented Nov 17, 2016

I just didn't want to go back and change anything later. Any plans for a release?

@zapov
Copy link
Member

zapov commented Nov 17, 2016

I'll look into releasing it over the weekend

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

2 participants