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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve RecentEra docs #4296

wants to merge 1 commit into from

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.

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