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.

Sign up for GitHub

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

Jump to bottom

Document GADT constructor arguments #539

Closed
wants to merge 1 commit into from
Closed

Document GADT constructor arguments #539

wants to merge 1 commit into from

Conversation

newhoggy
Copy link
Collaborator

Changelog

- description: |
    Document GADT constructor arguments
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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

@newhoggy newhoggy force-pushed the newhoggy/document-gadt-constructor-arguments branch from 4478883 to 11db5cf Compare May 20, 2024 14:32
carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Certificate.hs Outdated
:: ConwayEraOnwards era
-> Ledger.ConwayTxCert (ShelleyLedgerEra era)
-> Certificate era
-- Pre-Conway
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we turn it into haddock?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand what this list means.

Copy link
Contributor

@carbolymer carbolymer May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not starting with -- | so this comment is not a haddock comment.

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Certificate.hs Outdated
-- ^ Shelley ledger transaction certificate
-> Certificate era

-- Conway onwards
Copy link
Contributor

@carbolymer carbolymer May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same as for the other constructor? I guess it wouldn't be a very informative haddock.

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Body.hs Outdated
@@ -798,12 +801,15 @@ deriving instance Eq (TxInsCollateral era)
deriving instance Show (TxInsCollateral era)

data TxInsReference build era where
-- ^ No transaction inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- ^ No transaction inputs
-- | No transaction inputs

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Body.hs Outdated
@@ -914,26 +923,30 @@ prettyRenderTxOut (TxOutInAnyEra _ (TxOut (AddressInEra _ addr) txOutVal _ _)) =
<> renderValue (txOutValueToValue txOutVal)

data TxReturnCollateral ctx era where

-- ^ No return collateral
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- ^ No return collateral
-- | No return collateral

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Body.hs Outdated
-> TxReturnCollateral ctx era

deriving instance Eq (TxReturnCollateral ctx era)
deriving instance Show (TxReturnCollateral ctx era)

data TxTotalCollateral era where

-- ^ No total collateral
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- ^ No total collateral
-- | No total collateral

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Body.hs Outdated
@@ -1140,13 +1192,18 @@ deriving instance Show (TxUpdateProposal era)

data TxMintValue build era where

TxMintNone :: TxMintValue build era
-- ^ No minting
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- ^ No minting
-- | No minting

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Sign.hs Outdated
Comment on lines 314 to 312
-- + transaction metadata (in Shelley and later)
-- + auxiliary scripts (in Allegra and later)
Copy link
Contributor

@carbolymer carbolymer May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
-- + transaction metadata (in Shelley and later)
-- + auxiliary scripts (in Allegra and later)
-- - transaction metadata additionally (in Shelley and later)
-- - auxiliary scripts additionally (in Allegra and later)

I think the lists can only be with dashes and stars https://hackage.haskell.org/package/haddock-cheatsheet-0.1.0.1/docs/Doc-Haddock.html#g:9
... or you can add plus signs after stars for example

-- - + transaction metadata (in Shelley and later)

carbolymer
carbolymer reviewed May 20, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Tx/Sign.hs Outdated
-> Alonzo.TxDats (ShelleyLedgerEra era)
-> Alonzo.Redeemers (ShelleyLedgerEra era)
-> TxBodyScriptData era
-- ^ No script data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- ^ No script data
-- | No script data

carbolymer
carbolymer approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Nice! Have you tested it with haddock command? For example: https://github.com/IntersectMBO/cardano-api/blob/main/.github/workflows/github-page.yml#L33
(this workflow takes 40mins so it's not triggered on PRs)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 5, 2024

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Jul 5, 2024
@palas palas requested a review from a team as a code owner July 5, 2024 18:00
@palas
Copy link
Contributor

palas commented Jul 5, 2024

FYI: Because we now enforce the use of cabal-gild to format cabal files and, in order to reduce the amount of hassle, I have cherry-picked the appropriate commit into your PR. This will make the cabal-gild CI check pass for the PR. The commit should be discarded automatically as soon as you either rebase or merge the PR (since it is already in the main branch). Feel free to discard it if you want, but that will make the new required action to fail until you rebase.

@palas palas force-pushed the newhoggy/document-gadt-constructor-arguments branch 2 times, most recently from b23368b to 94a9c1d Compare July 5, 2024 18:10
@github-actions github-actions bot removed the Stale label Jul 6, 2024
@palas
Copy link
Contributor

palas commented Jul 6, 2024

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch here: backup/newhoggy/document-gadt-constructor-arguments

@palas palas force-pushed the newhoggy/document-gadt-constructor-arguments branch 3 times, most recently from d480c63 to 3d95c7d Compare July 6, 2024 03:46
@palas palas force-pushed the newhoggy/document-gadt-constructor-arguments branch from 3d95c7d to 9cb8d6d Compare July 6, 2024 04:00
@palas palas force-pushed the newhoggy/document-gadt-constructor-arguments branch from 9cb8d6d to 454a4e4 Compare July 11, 2024 16:21
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Aug 26, 2024
@github-actions GitHub Actions
Copy link

This issue was closed because it has been stalled for 60 days with no activity.

@github-actions github-actions bot closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350

@erikd erikd Awaiting requested review from erikd

@smelc smelc Awaiting requested review from smelc

@palas palas Awaiting requested review from palas

@kevinhammond kevinhammond Awaiting requested review from kevinhammond

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@disassembler disassembler Awaiting requested review from disassembler

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @palas @carbolymer