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

Added examples of transaction creation to Haddock #698

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Added examples of transaction creation to Haddock #698

merged 1 commit into from
Feb 21, 2025
+947 −172

Conversation

palas
Copy link
Contributor

@palas palas commented Dec 3, 2024 •
edited
Loading

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

@palas palas self-assigned this Dec 3, 2024
smelc
smelc approved these changes Dec 5, 2024
View reviewed changes
Jimbo4350
Jimbo4350 reviewed Dec 9, 2024
View reviewed changes
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.

@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
Jimbo4350
Jimbo4350 reviewed Dec 23, 2024
View reviewed changes
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 👍

@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 •
edited
Loading

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

@palas palas force-pushed the haddock-for-experimental-api branch from f658862 to 25e9c36 Compare January 7, 2025 11:22
@palas palas mentioned this pull request Feb 6, 2025
Merged
3 tasks
@palas palas force-pushed the haddock-for-experimental-api branch 3 times, most recently from ab519b2 to cec9101 Compare February 11, 2025 17:56
Jimbo4350
Jimbo4350 reviewed Feb 12, 2025
View reviewed changes
Jimbo4350
Jimbo4350 requested changes Feb 12, 2025
View reviewed changes
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.

Most of the links to modules are broken. We need to fix this. I also need you to work with @olgahryniuk to improve the flow of the documentation.

@palas palas force-pushed the haddock-for-experimental-api branch from 7d3ba15 to a083ac2 Compare February 13, 2025 10:38
@carbolymer
Copy link
Contributor

carbolymer commented Feb 13, 2025 •
edited
Loading

FYI https://github.com/IntersectMBO/cardano-api/actions/workflows/github-page.yml has a trigger, where you can specify a branch. It'll produce an artifact with haddocks e.g.: https://github.com/IntersectMBO/cardano-api/actions/runs/13306164579

olgahryniuk
olgahryniuk approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@olgahryniuk olgahryniuk left a comment

Choose a reason for hiding this comment

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

Several typos to fix, please.

olgahryniuk
olgahryniuk reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@olgahryniuk olgahryniuk left a comment

Choose a reason for hiding this comment

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

Final suggestions

@palas palas requested a review from Jimbo4350 February 20, 2025 13:23
Jimbo4350
Jimbo4350 approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍 . A few comments need to be addressed and the commits should be squashed.

This was referenced Feb 21, 2025
Open
Open
@palas
Copy link
Contributor Author

palas commented Feb 21, 2025

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

#760

@palas
Copy link
Contributor Author

palas commented Feb 21, 2025

https://github.com/IntersectMBO/cardano-api/tree/backup/haddock-for-experimental-api

@palas palas force-pushed the haddock-for-experimental-api branch from a1bcfa7 to 13ac778 Compare February 21, 2025 16:16
@palas
Add Haddock for traditional and experimental API
7c976f6 
Co-authored-by: olgahryniuk
<67585499+olgahryniuk@users.noreply.github.com>
@palas palas force-pushed the haddock-for-experimental-api branch from 13ac778 to 7c976f6 Compare February 21, 2025 16:35
@palas palas enabled auto-merge February 21, 2025 16:36
@palas palas added this pull request to the merge queue Feb 21, 2025
@Jimbo4350 Jimbo4350 removed this pull request from the merge queue due to a manual request Feb 21, 2025
@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Feb 21, 2025 •
edited
Loading

This comment has not be addressed: #698 (comment)

Apologies it has

@Jimbo4350 Jimbo4350 self-requested a review February 21, 2025 17:08
Jimbo4350
Jimbo4350 previously requested changes Feb 21, 2025
View reviewed changes
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.

I guess I was not clear enough. Address the comments please before merging.

@Jimbo4350 Jimbo4350 self-requested a review February 21, 2025 17:11
@Jimbo4350 Jimbo4350 enabled auto-merge February 21, 2025 17:11
@Jimbo4350 Jimbo4350 dismissed their stale review February 21, 2025 17:12

Everything has been addressed

@Jimbo4350 Jimbo4350 added this pull request to the merge queue Feb 21, 2025
Merged via the queue into master with commit 50702e6 Feb 21, 2025
29 checks passed
@Jimbo4350 Jimbo4350 deleted the haddock-for-experimental-api branch February 21, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olgahryniuk olgahryniuk olgahryniuk approved these changes

@smelc smelc smelc approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

@erikd erikd Awaiting requested review from erikd erikd is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

@carbolymer carbolymer Awaiting requested review from carbolymer carbolymer is a code owner

Assignees

@palas palas

Labels
documentation Improvements or additions to documentation test Adding or reworking tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@palas @carbolymer @Jimbo4350 @smelc @olgahryniuk