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

Add missing Show instances #607

Closed
ceedubs opened this issue Nov 7, 2015 · 8 comments
Closed

Add missing Show instances #607

ceedubs opened this issue Nov 7, 2015 · 8 comments

Comments

@ceedubs
Copy link
Contributor

ceedubs commented Nov 7, 2015

Many data types already have Show instances (and more will after #600, #601, and #602 are merged). However, there are still a few that are missing. Two that I see at the moment are Prod and WriterT. We should add those and any others we can find that are missing.

Edit: removed Prod from the scope of this issue per https://github.com/non/cats/issues/607#issuecomment-155074440.

@mikejcurry
Copy link
Contributor

I was going to add an instance for Prod; and learn the functionality at the same time, but it turned out to be a bit like peeling an onion. Less the tears part and more the layers part, but... :-)

Anyway, the thing with Prod specifically, and maybe this deserves its own issue, is that it is constructed to be lazy and it feels wrong for the Show instance to bypass this and compute the value. So, I figured an Show instance for Eval which respected the laziness of Eval might be useful.

Before I go too deep down this road, the question I have is whether a Show for Eval makes sense. E.g. for something like Now it would show Now(value) and something like Later it could show the value if already computed and could show something like Later(() => ResultType) if the value has not been computed.

The next layer of the onion is that there are Show instances for BigInt and BigDecimal but not for primitives such as Int, Char etc (as far as I could see) - does it make sense to add these also?

My feeling is that all of these make sense at some level, and that Show is pretty much universally applicable as you may want to Show pretty much anything.

Let me know what you think.

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 8, 2015

@mikejcurry I'm about to go somewhere, but there's one simple part of this that I can address first :). The Show instances for primitives are here.

@mikejcurry
Copy link
Contributor

Aah ok, I missed those for some reason. Sorry about that.

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 9, 2015

@mikejcurry Thank you for brining up the rabbit hole you went down with Prod. I agree that it's awkward, and I've created #615 to address that. Let's drop Prod from the scope of this issue.

@matt-martin
Copy link

I've tried to add a Show instance for WriterT with the following pull request: #928. Just trying to get my feet wet here with some "low-hanging fruit" so I'd welcome any and all feedback.

stew added a commit that referenced this issue Mar 16, 2016
@EncodePanda
Copy link

Since #928 is merged, shouldn't this issue be closed?

@ceedubs
Copy link
Contributor Author

ceedubs commented May 11, 2016

@rabbitonweb I suspect there are more missing, but this is a pretty broad/vague issue, so if we find any we should probably create a separate issue for them. Thanks for the heads up - I'll close this out.

@ceedubs ceedubs closed this as completed May 11, 2016
@ceedubs ceedubs removed the ready label May 11, 2016
@EncodePanda
Copy link

@ceedubs I'm sort of looking for a low hanging fruit - an entrypoint to contribution to cats, maybe if u could find one for, creating a Show instance would be a good starting point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants