Add guice7 (jakarta.inject) module #209
Conversation
|
|
josephlbarnett
force-pushed
the
guice7
branch
2 times, most recently
from
ec86650 to
f85aa7e
Compare
Copy guice (javax.inject) module over to a new guice 7 (jakarta.inject) module. Replace javax imports with jakarta imports, and leave everything else the same. Fixes FasterXML#206
| //Jakarta Reference Implementation | ||
| requires static jakarta.inject; | ||
|
|
||
| requires com.fasterxml.jackson.annotation; |
There was a problem hiding this comment.
transitive on com.fasterxml.jackson.annotation? ok, i'll defer to you on module-info stuff but want to be sure I get it right. should that change also be made to the existing guice module?
There was a problem hiding this comment.
If change was made here, yes, I think similarly to existing. I don't have strong opinion.
| } | ||
| } | ||
|
|
||
| private static class ObjectMapperProvider implements Provider<ObjectMapper> |
There was a problem hiding this comment.
can this be split out, to allow overriding on the bindings, or to rescope to object mapper
There was a problem hiding this comment.
@cowtowncoder Would this be ok? Allowing to provide the object mapper as a singleton to beat around the LRU cache?
There was a problem hiding this comment.
possibly, but not sure exactly what that would look like. feels like a separate PR against existing guice module (and this new one if timing of that PR is after this gets in) would make more sense to me?
There was a problem hiding this comment.
I think that (possible separate follow-up PR) makes sense assuming this is not a deviation from existing Guice module.
There was a problem hiding this comment.
Yeah agreed - will get started with that (y)
|
Sounds good, happy to merge (we have CLA). Just one question: I guess "guice7" works (and I think it's best to leave existing id of "guide" etc as-is for compatibility), but I wonder if alternative of "guice-jakarta" would make sense wrt later Guice versions? (since this would work for Guice 8.x etc). I don't have strong opinion here but many other modules add "jakarta". |
|
I dont have a strong opinion either way -- I did notice the latest guice main branch commit removes java8 support, so not sure if that changes anything about how you'd think about these guice modules? |
Hmmh. That gets more difficult wrt keeping Guice module here while building others for Java 8 since ideally we wouldn't use later JDKs for building (just to avoid accidental beyond-JDK-8 deps for modules not meant to require later baseline). So let's go with |
|
@josephlbarnett One question before merging -- have you addressed @GedMarc's concerns? If not, if you could do that (make change if they make sense which probably does for module-info -- or add a note why change doesn't make sense). |
| } | ||
| } | ||
|
|
||
| private static class ObjectMapperProvider implements Provider<ObjectMapper> |
There was a problem hiding this comment.
@cowtowncoder Would this be ok? Allowing to provide the object mapper as a singleton to beat around the LRU cache?
| } | ||
| } | ||
|
|
||
| private static class ObjectMapperProvider implements Provider<ObjectMapper> |
There was a problem hiding this comment.
Yeah agreed - will get started with that (y)
| @@ -0,0 +1,114 @@ | |||
| package com.fasterxml.jackson.module.guice; | |||
There was a problem hiding this comment.
Whops. guice -> guice7; will fix.
There was a problem hiding this comment.
oops sorry about that, thanks for fixing!!
There was a problem hiding this comment.
np @josephlbarnett ! Just happy I noticed it :)
|
Merged; also converted |
|
@cowtowncoder any chance of putting this in a 2.15.3 on a quicker timeline than a 2.16 release? is there a timeline on 2.16? (if not, no problem, just curious!) |
|
@josephlbarnett I do not, as a general rule, add new modules in patch releases: it's against SemVer and can be confusing. No specific timeline for 2.16; I need to send an email since this time around I have specific idea of what needs to be ready for 2.16 -- and as soon as those things are in I'm happy to do RC. |
Copy guice (javax.inject) module over to a new guice 7 (jakarta.inject) module. Replace javax imports with jakarta imports, and leave everything else the same.
Fixes #206