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

Added examples of transaction creation to Haddock #698

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

palas
Copy link
Contributor

@palas palas commented Dec 3, 2024

Changelog

- description: |
    Added example of transaction creation with traditional and experimental APIs to Haddock.
  type:
  - test
  - documentation

Context

Prompted by this slack discussion

How to trust this PR

Have a read of the haddock.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
Copy link
Contributor

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

Nice work 👍 . I would move the transaction creation haddocks to Cardano.Api.Experimental.Tx and point to it from here.

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
-- let sbe = Api.shelleyBasedEra :: Api.ShelleyBasedEra Api.ConwayEra
-- @
--
-- We can also derive it from 'ConwayEra' from 'Cardano.Api.Experimental' by using the 'convert' function:
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this is a bad UI. We should have a defaultTxBodyContent defined in an experimental module:

defaultTxBodyContent
  :: ()
  => Era era
  -> TxBodyContent BuildTx era

We want to avoid mixing the old api and the new api (but obviously allow for migration e.g via convert).

These haddocks are excellent but what we should do is have haddocks strictly describing the old api, the new api and how to migrate from the old to the new.

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
@palas palas force-pushed the haddock-for-experimental-api branch from ec017ee to bd4f6ba Compare December 11, 2024 11:33
@palas palas added documentation Improvements or additions to documentation test Adding or reworking tests labels Dec 11, 2024
@palas palas force-pushed the haddock-for-experimental-api branch from 0ddace3 to c9d390f Compare December 13, 2024 14:36
Copy link
Contributor

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

Getting there 👍

cardano-api/internal/Cardano/Api/Tx/Body.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Experimental/Tx.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Experimental/Tx.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Fees.hs Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
@carbolymer
Copy link
Contributor

Outside of the scope of this PR: Maybe could use doctest to make sure that haddock examples didn't bitrot?

@palas
Copy link
Contributor Author

palas commented Jan 7, 2025

Outside of the scope of this PR: Maybe could use doctest to make sure that haddock examples didn't bitrot?

Yes, I was thinking the same. Currently it is not checked by the CI, but at least it is easy to update if we do. And I made a couple of examples using this format in a different PR already (unrelated).

Co-authored-by: Mateusz Galazyn <[email protected]>
@palas palas force-pushed the haddock-for-experimental-api branch from f658862 to 25e9c36 Compare January 7, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation test Adding or reworking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants