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

Added debug instances for Tuple 3 to 22 #61

Merged
merged 1 commit into from
Apr 3, 2020
Merged

Added debug instances for Tuple 3 to 22 #61

merged 1 commit into from
Apr 3, 2020

Conversation

jaliss
Copy link
Collaborator

@jaliss jaliss commented Mar 31, 2020

No description provided.

@adamgfraser
Copy link
Contributor

Looks good! See my comment on #58 regarding the tuple names.

@jaliss
Copy link
Collaborator Author

jaliss commented Apr 1, 2020

@adamgfraser I think you have a point on the tuple names. I also like it better that there would be an explicit representation for tuples instead of using VConstructor. The renderer can take advantage of this later and decide what to do with the tuples.

If you agree I'll modify things to look like:

final case class Tuple2[A, B](value: (A, B)) extends Repr

implicit def Tuple2Debug[A: Debug, B: Debug]: Debug[(A, B)] = tuple => Repr.Tuple2((tuple._1.debug, tuple._2.debug))

@adamgfraser
Copy link
Contributor

@jaliss I think that could make sense.

Technically the renderer could decide either way just by looking at the parameters of VConstructor and checking if it they are List("scala", "Tuple2"), etc... So creating a separate case for this is a little bit of a trade-off between us doing more work to make more subtypes versus making it a little easier for the renderer (it can pattern match on the subtypes instead of having to check the strings directly).

There is also a little bit of a slippery slope issue of how we decide whether to create a new subtype versus using VConstructor (e.g. should we have subtypes for List, Map, etc...).

Maybe we say we have subtypes for the "standard" types that we have other type classes for and use VConstructor for everything else.

If we do create a separate subtype I would suggest we just have one Tuple subtype that takes a list of parameters so we don't need to have 22 separate cases, which also might point towards just using VConstructor.

@jaliss
Copy link
Collaborator Author

jaliss commented Apr 3, 2020

@adamgfraser I think your original concern on the tuple names can be addressed in the renderer so I would keep the name (eg: Tuple2). It would be up to the renderer to decide if it wants to include the name or just produce a (a, b) representation.

VConstructor seems to be good enough for now, so maybe let's keep the number of Repr instances to a minimum until we really see there's a need for more.

@adamgfraser
Copy link
Contributor

@jaliss Sounds good.

Copy link
Contributor

@adamgfraser adamgfraser left a comment

Choose a reason for hiding this comment

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

We can merge now and come back to the issue of potentially have a more structured representation of some standard library types later.

@adamgfraser adamgfraser merged commit f2053e6 into zio:master Apr 3, 2020
@jaliss jaliss deleted the debug_tuple branch April 3, 2020 13:09
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.

None yet

2 participants