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

Provide concise .toString for free structures. #1084

Merged
merged 2 commits into from
Jun 3, 2016

Conversation

non
Copy link
Contributor

@non non commented Jun 2, 2016

Previously calling .toString on a Free value would traverse
the entire ADT of case classes. In some cases this would
actually cause a stack overflow in the printing code. It
also represented a (small) law violation (you could use the
output to distinguish x and x.map(identity) in some cases).

This change replaces the output with opaque "Free(...)"
and "FreeApplicative(...)" strings.

Previously calling .toString on a Free value would traverse
the entire ADT of case classes. In some cases this would
actually cause a stack overflow in the printing code. It
also represented a (small) law violation (you could use the
output to distinguish x and x.map(identity) in some cases).

This change replaces the output with opaque "Free(...)"
and "FreeApplicative(...)" strings.
@non non added the in progress label Jun 2, 2016
@codecov-io
Copy link

codecov-io commented Jun 2, 2016

Current coverage is 88.12%

Merging #1084 into master will decrease coverage by <.01%

@@             master      #1084   diff @@
==========================================
  Files           224        224          
  Lines          2842       2844     +2   
  Methods        2785       2787     +2   
  Messages          0          0          
  Branches         52         52          
==========================================
+ Hits           2505       2506     +1   
- Misses          337        338     +1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 7f112dd...7bd86af

@mandubian
Copy link
Contributor

👍

1 similar comment
@fthomas
Copy link
Member

fthomas commented Jun 3, 2016

👍

@ceedubs
Copy link
Contributor

ceedubs commented Jun 3, 2016

@non would you be willing to add a regression test that a deeply-nested Free doesn't result in a stack overflow when toString is called?

@non
Copy link
Contributor Author

non commented Jun 3, 2016

@ceedubs added.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 3, 2016

Thanks @non! 👍 as soon as the build passes.

@ceedubs ceedubs merged commit 3608287 into typelevel:master Jun 3, 2016
@ceedubs ceedubs mentioned this pull request Aug 13, 2016
8 tasks
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.

6 participants