Skip to content

Conversation

@carbolymer
Copy link
Collaborator

No description provided.

@carbolymer carbolymer self-assigned this Feb 7, 2025
@carbolymer carbolymer force-pushed the mgalazyn/adr-cardano-api-top-level-exports-convention branch 2 times, most recently from 72d1316 to fe9bba4 Compare February 10, 2025 11:33
Copy link
Collaborator

@palas palas left a 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
Copy link
Collaborator

@Jimbo4350 Jimbo4350 Feb 10, 2025

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.

Copy link
Collaborator Author

@carbolymer carbolymer Feb 11, 2025

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:
Copy link
Collaborator

@Jimbo4350 Jimbo4350 Feb 10, 2025

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.

@carbolymer carbolymer force-pushed the mgalazyn/adr-cardano-api-top-level-exports-convention branch 3 times, most recently from 77d2f6d to e165f8c Compare February 14, 2025 11:46

* 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`.
Copy link
Collaborator

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

Copy link
Collaborator Author

@carbolymer carbolymer Feb 21, 2025

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 API
  • Cardano.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.

Copy link
Collaborator

@Jimbo4350 Jimbo4350 left a 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

@carbolymer
Copy link
Collaborator Author

@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?

@carbolymer carbolymer force-pushed the mgalazyn/adr-cardano-api-top-level-exports-convention branch from e165f8c to 2c0c053 Compare February 21, 2025 14:38
@carbolymer carbolymer changed the title ADR X: cardano-api modules exports convention ADR 9: cardano-api modules exports convention Feb 21, 2025
@carbolymer carbolymer marked this pull request as ready for review February 21, 2025 14:39
@carbolymer carbolymer requested review from a team as code owners February 21, 2025 14:39
@carbolymer carbolymer force-pushed the mgalazyn/adr-cardano-api-top-level-exports-convention branch from 2c0c053 to 06e26a6 Compare February 24, 2025 12:41
@carbolymer carbolymer merged commit 1ff0dd8 into main Feb 24, 2025
1 check failed
@carbolymer carbolymer deleted the mgalazyn/adr-cardano-api-top-level-exports-convention branch February 24, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants