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

debug #58

Merged
merged 16 commits into from Apr 6, 2020
Merged

debug #58

merged 16 commits into from Apr 6, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 31, 2020

#1

I was trying to add both and bothWith and Either for this one. but im not sure how to compose two Reprs.

@adamgfraser
Copy link
Contributor

@sken77 This looks like a good start.

I don't think we can implement bothWith or eitherWith for Debug, at least in their pure forms. The issue is that Debug requires too much information that is not captures in the types of the elements. For example, consider:

final case class Engineer(name: String, salary: Int)
final case class Architect(name: String, salary: Int)

For something like Equal if we have a product type with two elements we can always compare it for equality if we have a way to compare each of the elements. It doesn't matter if it is a Engineer or an Architect if we can compare names and salaries we can say whether one engineer is equal to another or one architect is equal to another.

But this isn't true for Debug because we also need to know the prefix. We could require the user to provide another parameter for the prefix and use that with Repr.VConstructor and similarly require the user to provide a prefix for the different cases in eitherWith. May be worth thinking about how we could provide combinators like that to make it more ergonomic for users to construct instances but we should probably not use the bothWith and eitherWith names because of these differences.

@ghost
Copy link
Author

ghost commented Mar 31, 2020

Yeah thats what i thought. I'll finish the rest of the Tuples with something similar i did for Tuple2.

@ghost
Copy link
Author

ghost commented Mar 31, 2020

So i just talked with @jaliss . he will add the Tuples on a different PR. so this one is good to go. @adamgfraser I think the other point to that the law of debug is symmetry? i recall that @jdegoes said on class that we should be parse the output into scala code. but that seems to be a much bigger issue outside of the scope of this issue

@adamgfraser
Copy link
Contributor

Yes, I think adding full support for that is going to require adding some functionality for doing runtime compilation which I think is doable but agree is outside the scope of this PR.

@adamgfraser
Copy link
Contributor

@sken77 Could we add a default renderer and some basic tests that rendering simple values results in the expected string?

@ghost
Copy link
Author

ghost commented Mar 31, 2020

Sure thing

@adamgfraser
Copy link
Contributor

Awesome!

@ghost
Copy link
Author

ghost commented Mar 31, 2020

Im having issues with the GenList test. for some reason its generating 4 different lists. thats why the test isnt passing. I post this progress just to check the toString call looks ok

@adamgfraser
Copy link
Contributor

@sken77 Will take a look in a little while.

@ghost
Copy link
Author

ghost commented Apr 1, 2020

#5 this pr also deals with this one

@ghost
Copy link
Author

ghost commented Apr 1, 2020

huh thats weird. fmt passed on my end

@ghost
Copy link
Author

ghost commented Apr 1, 2020

I cant see the details since its prolly private in Circle

@jdegoes
Copy link
Member

jdegoes commented Apr 4, 2020

@sken77 Can you resolve conflicts and make sure tests pass manually, then we can get this one merged in?

@ghost
Copy link
Author

ghost commented Apr 4, 2020

@jdegoes ok this is good to go

@ghost ghost changed the title debug first pass debug Apr 4, 2020
@ghost ghost requested a review from jdegoes April 5, 2020 22:41
@jdegoes jdegoes merged commit bb8e7bf into zio:master Apr 6, 2020
@jdegoes
Copy link
Member

jdegoes commented Apr 6, 2020

@sken77 Thank you!

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

3 participants