search

rharter / auto-value-gson

AutoValue Extension to add Gson De/Serializer support
Apache License 2.0
607 stars 103 forks source link

Make generated class + constructor package private #228

Closed ZacSweers closed 4 years ago

ZacSweers commented 5 years ago

The runtime change is backward compatible to old generated code, but new generated code requires the new runtime version. I feel like it's acceptable, but we should call this out in the changelog. Lmk your thoughts

rharter commented 5 years ago

I don't think this is quite right. It looks like the OP was referencing the Factory generated by the @GsonTypeAdapterFactory annotation, not the adapter generated by the @GenerateTypeAdapter annotation (the different mechanisms are quite confusing). It doesn't make sense for the adapter generated specifically to be accessed from the com.ryanharter.autovalue.gson package to be package private.

ZacSweers commented 5 years ago

Oh snap, you're right 😬. That said, any reason not to make this match the visibility of the model?

On Fri, Oct 18, 2019 at 9:59 PM Ryan Harter notifications@github.com wrote:

I don't think this is quite right. It looks like the OP was referencing the Factory https://github.com/rharter/auto-value-gson#factory generated by the @GsonTypeAdapterFactory annotation, not the adapter generated by the @GenerateTypeAdapter annotation (the different mechanisms are quite confusing). It doesn't make sense for the adapter generated specifically to be accessed from the com.ryanharter.autovalue.gson package to be package private.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rharter/auto-value-gson/pull/228?email_source=notifications&email_token=AAKMJPS5XWGVWFTHGR7WTB3QPJSYJA5CNFSM4JCNLHA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBWYQII#issuecomment-544049185, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKMJPX7477MGYOTDAUXUBDQPJSYJANCNFSM4JCNLHAQ .

rharter commented 5 years ago

I'm a bit curious why you'd want this, and the model, to be package private if you also want it accessible from the GenerateTypeAdapter.FACTORY class. Seems like setting visibility to something that doesn't match the intended use case just so the factory can reflectively modify the accessibility to violate that visibility.

Unless there's a reason that you'd want a top level adapter that doesn't involve using that factory, like you have your own factory in that package, but that violate's AutoValue's value of hiding the implementation details (the AutoValue_ generated classes) behind static factory methods so they're easily interchangeable.

Is there some use case I'm missing?

ZacSweers commented 5 years ago

This was the intended design when I wrote it (the internal version I wrote at Uber worked the same way iirc), with the intention that reflection handles the package private access and allow the adapter itself to remain an implementation detail as far as other source code is concerned.

ZacSweers commented 5 years ago

Yeah for somewhere like Moshi it made sense to keep them public or match visibility since there's no factory option, but in this case the thought was that if you're using the factory support then you're already not using the @GenerateTypeAdapter annotation. Does that sound clearer?

rharter commented 4 years ago

Yeah, that works for me.