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

Improve RecentEra docs #4296

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Improve RecentEra docs #4296

merged 2 commits into from
Dec 18, 2024

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Dec 4, 2023

@Anviking Anviking self-assigned this Dec 4, 2023
@Anviking Anviking force-pushed the anviking/RecentEra-docs branch from e936093 to 54a5602 Compare December 4, 2023 18:47
@abailly
Copy link
Collaborator

abailly commented Dec 11, 2024

Is this still relevant?

@Anviking Anviking marked this pull request as ready for review December 11, 2024 17:05
@Anviking Anviking force-pushed the anviking/RecentEra-docs branch from 54a5602 to 1a0fa53 Compare December 11, 2024 17:10
@Anviking
Copy link
Member Author

IMO RecentEra would still deserve some improved docs.

@HeinrichApfelmus do you think we could agree on some variation of this without too much difficulty?

This example in particular is currently in conflict with the decision

myCardanoApiTx :: IsRecentEra era -> RecentEra -> CardanoApi.Tx (CardanoApiEra era)

had concerns about it, don't remember if resolved, but can ignore for now, could copy the examples from the decision doc exactly as a first step.

Comment on lines 128 to 129
-- renderCliTx :: IsRecentEra era -> Tx era -> Text
-- myCardanoApiTx :: IsRecentEra era -> RecentEra -> CardanoApi.Tx (CardanoApiEra era)
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
-- renderCliTx :: IsRecentEra era -> Tx era -> Text
-- myCardanoApiTx :: IsRecentEra era -> RecentEra -> CardanoApi.Tx (CardanoApiEra era)
-- renderCliTx :: IsRecentEra era => Tx era -> Text
-- myCardanoApiTx :: IsRecentEra era => RecentEra era → CardanoApi.Tx (CardanoApiEra era)

The other variants do not type check, I believe.

@HeinrichApfelmus
Copy link
Contributor

This example in particular is currently in conflict with the decision

myCardanoApiTx :: IsRecentEra era -> RecentEra -> CardanoApi.Tx (CardanoApiEra era)

had concerns about it, don't remember if resolved,

I think the specific trouble with myCardanoApiTx might have been that CardanoApiEra is not an injective type family, and specifying the return type is not enough to determine era — this type would be ambiguous.

In general, however, the decision document does imply that era in the return value is sufficient cause to not have have it as an explicit argument. For example,

parseThisRecentEra :: IsRecent era => ByteString  Maybe (RecentEra era)

is preferred over

parseThisRecentEra' :: IsRecent era => RecentEra era  ByteString  Maybe (RecentEra era)

but can ignore for now, could copy the examples from the decision doc exactly as a first step.

Yes, please. Copying the examples from the decision document exactly is a good idea.

@Anviking Anviking force-pushed the anviking/RecentEra-docs branch 2 times, most recently from f228513 to b02df03 Compare December 17, 2024 14:03
@Anviking Anviking force-pushed the anviking/RecentEra-docs branch from b02df03 to ad66d40 Compare December 17, 2024 14:05
@Anviking
Copy link
Member Author

@HeinrichApfelmus I've updated the 'RecentEra' the value vs 'IsRecentEra' the class section to exactly mirror the advice process decision ad66d40

Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Thanks! 😊 👍

@Anviking Anviking added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@Anviking Anviking added this pull request to the merge queue Dec 18, 2024
Merged via the queue into master with commit c010fdb Dec 18, 2024
24 checks passed
@Anviking Anviking deleted the anviking/RecentEra-docs branch December 18, 2024 14:13
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.

3 participants