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

Support mechanism to exclude some classes #17

Closed
lenguyenthanh opened this issue Mar 24, 2016 · 17 comments
Closed

Support mechanism to exclude some classes #17

lenguyenthanh opened this issue Mar 24, 2016 · 17 comments

Comments

@lenguyenthanh
Copy link

Default behavior of this extension is create TypeAdapterFactory for every AutoValue classes. But maybe not all of AutoValue classes need it. So it'd be great if we have a mechanism to exclude or include a specific AutoValue class. A @gson Annotation is a good idea.

@lenguyenthanh
Copy link
Author

@rharter do you have any idea for this feature? is it good? I'm happy to make a PR. But wanna discuss with you first. Cheers!

@gabrielittner
Copy link
Contributor

I've just found another reason for this. The gson and moshi extensions don't support generics in AutoValue classes and generate code that won't compile for those. So without opt-in/out you either can't use generics or can't use the gson/moshi extension.

@rharter
Copy link
Owner

rharter commented Mar 30, 2016

I do think this is a good idea, I'm just trying to come up with a good way to handle it. We need to play nice with other extensions, so I'd love it if there was a way to include/exclude classes without the need for another extension, but I'm not sure there is.

Are you suggesting an explicit @Gson annotation for inclusion?

@AutoValue @Gson public abstract class Foo {

}

@lenguyenthanh
Copy link
Author

Yes, it is exactly what I propose. I'd like to add a runtime Module for @Gson annotation only. On main extension library, it should start generate code only if a class has @Gson annotation.

@gabrielittner
Copy link
Contributor

I think there are two completely different usage scenarios. You either want it to run on all and exclude few or the opposite. Adding @Gson to all your classes can get very annoying and using the static TypeAdapterFactory method as opt-in signal won't work with auto-registration.

What do you think of having two separate artifacts? One works like the current one and has a @NoGson annotation to opt-out. The other one requires explicit opt-in by adding the static TypeAdapterFactory method or adding @Gson.

@rharter
Copy link
Owner

rharter commented Mar 30, 2016

First of all, there's no need for a runtime module, as even if there was a @Gson annotation, it would only need source retention and not be needed at runtime.

That being said, @gabrielittner's points are exactly my concerns. In your case, you want to exclude most classed, but explicitly include certain ones. Most of my cases are the opposite, in which most of my classes, benefit from having TypeAdapters. The challenge is that making things easier for one case makes them harder for the other.

As for the idea of having multiple artifacts, that sounds like a management headache and would be confusing to users, so I'm not a big fan of that idea.

@lenguyenthanh
Copy link
Author

I think two atifacts for two scenarios is quite strange. And it's hard to maintain two different codebases.

I still prefer @gson annotation option because it makes our code clearer, we have to explicit what classes are support Gson. Option 2, with @nogson anntation, adds magic to our code si nce it automatically adds suppoting Gson. And we can forget to add @gson easily when we adding new no gson classes.

@gabrielittner
Copy link
Contributor

I don't think managing would be a problem, one shared abstract base class and the two implementations would just have to implement applicable(). But you're right that it's probably too confusing.

@lenguyenthanh
Copy link
Author

There is only one problem with no runtime module is Android Studio cannot understand @Gson annotation when we use it with apt plugin. So we have to include the library with apt and provided. Something like this:

// Do not compile AutoValue dependencies to the app.
apt libraries.autoValue
// Make AutoValue annotation visible to the compiler.
provided libraries.autoValue

But I think it's IDE bug, so it may not effect to our decision.

@JakeWharton
Copy link
Contributor

It is not an IDE bug. Annotations should be separate from their compilers.

On Wed, Mar 30, 2016 at 11:23 PM Thanh Le notifications@github.com wrote:

There is only one problem with no runtime module is Android Studio cannot
understand @gson annotation when we use it with apt plugin. So we have to
include the library with apt and provided. Something like this:

// Do not compile AutoValue dependencies to the app.
apt libraries.autoValue
// Make AutoValue annotation visible to the compiler.
provided libraries.autoValue

But I think it's IDE bug, so it may not effect to our decision.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#17 (comment)

@lenguyenthanh
Copy link
Author

Thanks @JakeWharton for correting my mistake.

@artem-zinnatullin
Copy link

Hm, idk but I'd like to explicitly mark something with @Gson/etc to generate gson mapping. We have a lot of immutable classes and only some part of them needs json serialization.

Imagine you're reading a class and not even realizing that it's going to be jsonable.

@yogurtearl
Copy link

I prefer @gson over @nogson

@dimsuz
Copy link

dimsuz commented Apr 1, 2016

Would also love to see @Moshi :)

@JakeWharton
Copy link
Contributor

File a bug on https://github.com/rharter/auto-value-moshi/

@rharter
Copy link
Owner

rharter commented Apr 6, 2016

So I've been thinking about how to do this cleanly, and I think it aligns with something I've been wanting to add for a while.

The approach would basically be to require the annotated class to include a public static method returning TypeAdapter. If that exists, then we generate the TypeAdapter, otherwise we don't.

This means to include the class in the gson extension you'd need something like this:

@AutoValue public abstract class Foo {
  public abstract String a();
  public abstract String b();

  public static TypeAdapter typeAdapter() {
    return new AutoValue_Foo.TypeAdapter();
  }
}

This will also open up the opportunity to generate a single TypeAdapterFactory that will return the appropriate TypeAdapter for any auto-value-gson generated class.

I'm curious to hear any feedback on this idea.

@lenguyenthanh
Copy link
Author

I think it is a good idea. It explicit Gson type without adding another annotation.

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

7 participants