-
Notifications
You must be signed in to change notification settings - Fork 7
ADR 9: cardano-api modules exports convention #63
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
ADR 9: cardano-api modules exports convention #63
Conversation
72d1316 to
fe9bba4
Compare
palas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few ideas
| Each top level export should export distinct symbols. | ||
| This may depend on the case, for example: `Cardano.Api.Byron` symbols which are still in use in current eras, can be reexported through `Cardano.Api`. | ||
|
|
||
| * `Cardano.Api` - everything domain related for the currently used and upcoming eras |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we have turned Cardano.Api and Cardano.Api.Shelley into a dumping ground.
cardano-api/internal/Cardano/Api/* has a mass of modules. We should start by grouping modules under sensible directories. E.g any Serialise*.hs module can go into a Serialization folder. Eventually we can have several submodules with clear names.
e.g Cardano.Api.Serialization that re-exports all of the relevant serialization modules.
The distinction between Cardano.Api and Cardano.Api.Shelley is unclear. Because we have a split between Byron and all the other eras, I would suggest we eventually deprecate Cardano.Api.Shelley and use Cardano.Api as an "everything import" if possible. But not exporting what you mentioned below regarding utilities to avoid name clashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have a split between Byron and all the other eras, I would suggest we eventually deprecate Cardano.Api.Shelley and use Cardano.Api as an "everything import" if possible.
💯 👍🏻
| Those should **not** be reexported from `Cardano.Api`, because they would pollute symbol namespace, and are not required for using `cardano-api` core features but can be useful for the advanced users. | ||
| 1. Reexports from upstream libraries like consensus, network or ledger, also fall into this category e.g. `Cardano.Api.Ledger`. | ||
|
|
||
| Modules which are exposing general purpose classes and functions should go also into a separate module, and they not need to be reexported through `Cardano.Api`, for example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they not need to be reexported through
Cardano.Api
We are in a way re-visiting the problematic question that got us here in the first place. "What should go in Cardano.Api`?
The answer in my opinion is everything. We should group common functionality under a "top level" module (see Cardano.Api.Serialization) and simply re-expose all of these "top-level" modules via Cardano.Api. For users who only want specific functionality they can pick and choose which modules they want instead of importing Cardano.Api. This also benefits us because we by default export everything we want exposed via Cardano.Api. I think we can avoid name clashes by exporting the utilities separately like you suggested.
77d2f6d to
e165f8c
Compare
|
|
||
| * Reexports from upstream libraries like consensus, network or ledger, also fall into this category e.g. `Cardano.Api.Ledger`. | ||
|
|
||
| * Modules which are exposing general purpose classes and functions should go also into a separate module, and they don't need to be reexported through `Cardano.Api`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the problem with an "everything" module like Cardano.Api, it can't be everything. I'd like to demarcate this via the module name more clearly. What about something like:
Cardano.Api.Direct.UTxO -- Direct import modules (not re-exported)
or overall something like:
Cardano.Api/
Core/ -- re-exported through Cardano.Api
Direct/ -- must be imported directly
Internal/ -- implementation details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only need in this case an Internal module. Everything what is exposed only through Cardano.Api should go to Cardano.Api.Internal. For example, if our current module Cardano.Api.Internal.Fees is meant to be only exposed through Cardano.Api, it should be where it's now.
If a module is meant to be a top level export, not necessarily exposed through Cardano.Api, it should be a top level module. If our current Cardano.Api.Internal.Fees is meant to be a top level export only, it should be split into two:
Cardano.Api.Fees- public APICardano.Api.Fees.Internal- internal functions, not exposed
Nothing prevents reexporting selected symbols from Cardano.Api.Fees through Cardano.Api if necessary.
I've described it in "Deeper level exports" section.
Jimbo4350
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 . However we may have to expose more modules than we would like because we want their haddocks to be rendered cc: @palas
|
@Jimbo4350 Usually reexported symbols carry haddocks with them, so that should be fine. We can also create modules with just haddocks, reexporting what's needed for that purpose. @palas could you support us here with few examples of problematic modules / symbols? |
e165f8c to
2c0c053
Compare
2c0c053 to
06e26a6
Compare
No description provided.