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

JPMS open modules do not export packages #836

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Conversation

jponge
Copy link
Member

@jponge jponge commented Jan 30, 2022

We incorrectly assumed that open modules would export all packages, which is not the case.

  • Open modules allow for (deep) reflection to all classes, which is something we want to allow. We have no good reason right now to restrict reflection in Mutiny.
  • Exported packages need to be added manually, there is no wildcard. I did a selection of packages that shall be in the public API.
  • I fixed transitive 'requires' clauses: Reactive Streams is part of the Mutiny API, just like SmallRye Common Annotations.

We incorrectly assumed that open modules would export all packages, which is not the case.

- Open modules allow for (deep) reflection to all classes, which is something we want to allow.
  We have no good reason right now to restrict reflection in Mutiny.

- Exported packages need to be added manually, there is no wildcard.
  I did a selection of packages that shall be in the public API.

- I fixed transitive 'requires' clauses: Reactive Streams is part of the Mutiny API, just like
  SmallRye Common Annotations.
@jponge jponge linked an issue Jan 30, 2022 that may be closed by this pull request
@jponge jponge added this to the 1.4.0 milestone Jan 30, 2022
@jponge jponge added the bug Something isn't working label Jan 30, 2022
@jponge jponge requested a review from cescoffier January 30, 2022 21:16
@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #836 (36727cd) into main (34fd96d) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 36727cd differs from pull request most recent head f16f97b. Consider uploading reports for the commit f16f97b to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main     #836      +/-   ##
============================================
+ Coverage     89.08%   89.18%   +0.09%     
- Complexity     3052     3055       +3     
============================================
  Files           383      383              
  Lines         12185    12185              
  Branches       1547     1549       +2     
============================================
+ Hits          10855    10867      +12     
+ Misses          697      694       -3     
+ Partials        633      624       -9     
Impacted Files Coverage Δ
...io/smallrye/mutiny/groups/UniAndGroupIterable.java 100.00% <ø> (ø)
...mallrye/mutiny/helpers/queues/MpscLinkedQueue.java 90.32% <0.00%> (-8.07%) ⬇️
...operators/multi/builders/CollectionBasedMulti.java 93.54% <0.00%> (-3.23%) ⬇️
...mallrye/mutiny/operators/multi/FlatMapManager.java 95.83% <0.00%> (-2.78%) ⬇️
...lrye/mutiny/subscription/SerializedSubscriber.java 79.81% <0.00%> (-0.92%) ⬇️
...mallrye/mutiny/operators/multi/MultiFlatMapOp.java 85.10% <0.00%> (+0.30%) ⬆️
...a/io/smallrye/mutiny/helpers/BlockingIterable.java 87.03% <0.00%> (+0.92%) ⬆️
...operators/multi/processors/BroadcastProcessor.java 88.46% <0.00%> (+1.28%) ⬆️
...subscription/SwitchableSubscriptionSubscriber.java 97.60% <0.00%> (+1.59%) ⬆️
...java/io/smallrye/mutiny/helpers/Subscriptions.java 80.66% <0.00%> (+1.65%) ⬆️
... and 3 more

Copy link
Contributor

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just not sure of the operators package.

I would only expose packages where we have revapi configured, which should be our public API.

@cescoffier
Copy link
Contributor

AbstractUni and AbstractMulti are definitely part of the public API (advanced use case where you need to implement your own operator). We use them in Reactive Messaging. I know that the Cassandra Quarkus extension is also using them.

@jponge
Copy link
Member Author

jponge commented Jan 31, 2022

@cescoffier See the latest changes to align RevAPI and the module descriptor

@jponge
Copy link
Member Author

jponge commented Jan 31, 2022

  • Renamed a class in ...operators.multi: RevAPI is ok
  • Renamed a class in ...operators: RevAPI complains

@jponge jponge merged commit 1117c40 into main Jan 31, 2022
@jponge jponge deleted the bug/incorrect-jpms-descriptors branch January 31, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with the new mutiny module-info.java (1.3.1)
2 participants