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

Show instance for UtxoState #267

Closed
wants to merge 1 commit into from
Closed

Show instance for UtxoState #267

wants to merge 1 commit into from

Conversation

gabrielhdt
Copy link
Contributor

This PR introduces a Show instance for UtxoState, and is related to #266.

I'm not sure where to put the instance declaration: either I put it next to the definition of UtxoState, but it makes a cyclic dependency graph, or I put it next to the definition of prettyUtxoState, and it feels kind of lost.

@florentc
Copy link
Member

Check #265 for insight on the dependency cycle issue.

I am not in favor of re-introducing a Show instance for UtxoState that is based on PrettyCooked. See my opinion on issue #266

@gabrielhdt
Copy link
Contributor Author

Would you prefer, or agree with a standard deriving? Or a slightly arranged deriving?

@florentc
Copy link
Member

I think it is ultimately not a good idea to derive a Show instance because it makes it compulsory to provide a Show instance for datums. This is currently the case, datums are required to have a Show instance. But since the move to Pretty/PrettyCooked, datums are required to instantiate PrettyCooked and the Show constraint has become redundant (and the idea is to eventually remove it). I think the better approach is to solve #234 and provide convenient functions to use in the REPL that rely on prettyCookedOpt instead of show.

Copy link
Contributor

@0xd34df00d 0xd34df00d left a comment

Choose a reason for hiding this comment

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

One opinion I've seen elsewhere is that, in the presence of pretty-printing for a family of types, Show is largely needed only as "inverse" to Read, which we don't provide and don't need really.

So, depending on what the rest of the folks think, I'm also totally fine with just not having any Show UtxoState at all, and relying on pretty -printing explicitly.

@gabrielhdt
Copy link
Contributor Author

After discussion, the modifications brought in this pull request do not fit with the intended design.

@gabrielhdt gabrielhdt closed this Feb 15, 2023
@gabrielhdt gabrielhdt deleted the gh/showMeUtxoStates branch February 15, 2023 07:44
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