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

[MRESOLVER-298] Make javax.inject optional again #235

Merged
merged 2 commits into from
Jan 12, 2023

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jan 11, 2023

javax.inject is and should be optional (used only when Guice and/or Sisu is being used), but in case SL is being used no javax.inject should be needed.

This rule was violated in 1.9.0 commit 5566bd5 where javax.inject.Provider was used in conjuction with SL as well.

This fix introduces NameMappers helper static class that constructs name mappers, and javax.inject.Provider classes are used ONLY when in Guice/Sisu, are NOT used with conjunction of SL.


https://issues.apache.org/jira/browse/MRESOLVER-298

javax.inject is and should be optional (used only when Guice and/or
Sisu is being used), but in case SL is being used no javax.inject
should be needed.

This rule was violated in 1.9.0 commit 5566bd5
where javax.inject.Provider was used in conjuction with SL as well.

---

https://issues.apache.org/jira/browse/MRESOLVER-298
@cstamas cstamas self-assigned this Jan 11, 2023
@cstamas cstamas changed the title [MRESOLVER-298] Make javax.inject optional [MRESOLVER-298] Make javax.inject optional again Jan 11, 2023
@michael-o
Copy link
Member

Call me stupid, but what makes it optional now? That the providers don't appear in the annoations and this avoids the transitive dep to javax.inject?

@cstamas cstamas marked this pull request as ready for review January 11, 2023 20:26
@cstamas
Copy link
Member Author

cstamas commented Jan 11, 2023

Call me stupid, but what makes it optional now? That the providers don't appear in the annoations and this avoids the transitive dep to javax.inject?

No dependency or scope was changed, it was code only, here https://github.com/apache/maven-resolver/pull/235/files#diff-530cadfdf6c10eff31703c1c50c7121f9c72c64a4f55c156428c7cc1113e6950R77-R86 This private static method is used with default ctor (when SL is used) and expected Provider on classpath. This is not the case anymore.

Simple proof: the "demo snippets" now work from IDE (they have main method), while on current master they do NOT work due same issue (javax.inject is present in test scope).

@michael-o
Copy link
Member

Call me stupid, but what makes it optional now? That the providers don't appear in the annoations and this avoids the transitive dep to javax.inject?

No dependency or dep scope was changed, it was code, here https://github.com/apache/maven-resolver/pull/235/files#diff-530cadfdf6c10eff31703c1c50c7121f9c72c64a4f55c156428c7cc1113e6950R77-R86 This private static method is used with default ctor (when SL is used) and expected Provider on classpath. This is not the case anymore.

Simple proof: the "demo snippets" now work from IDE (they have main method), before this change, on current master they do NOT work due same issue (as javax.inject is only in test scope).

This is basically what I meant. The Provider had to be there...

@cstamas cstamas added this to the 1.9.4 milestone Jan 11, 2023
@cstamas cstamas requested a review from olamy January 11, 2023 20:52
@cstamas cstamas merged commit 1010f0f into apache:master Jan 12, 2023
@cstamas cstamas deleted the MRESOLVER-298-javax-inject branch January 12, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants