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.

Sign up for GitHub

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

Jump to bottom

String interpolator for Show, without macros #1495

Merged
merged 2 commits into from
Dec 21, 2016
Merged

String interpolator for Show, without macros #1495

merged 2 commits into from
Dec 21, 2016

Conversation

dwijnand
Copy link
Contributor

Non-macro alternative to #1489.

Happy to iterate - feedback welcome.

@dwijnand dwijnand mentioned this pull request Dec 12, 2016
Closed
@codecov-io
Copy link

codecov-io commented Dec 12, 2016 •
edited
Loading

Current coverage is 91.94% (diff: 100%)

Merging #1495 into master will increase coverage by 0.01%

@@             master      #1495   diff @@
==========================================
  Files           244        245     +1   
  Lines          3621       3626     +5   
  Methods        3502       3503     +1   
  Messages          0          0          
  Branches        119        123     +4   
==========================================
+ Hits           3329       3334     +5   
  Misses          292        292          
  Partials          0          0

Powered by Codecov. Last update 73a6481...54eeffe

TomasMikula
TomasMikula reviewed Dec 12, 2016
View reviewed changes
core/src/main/scala/cats/Show.scala
}

implicit class ShowInterpolator(val _sc: StringContext) extends AnyVal {
def show(args: Shown*): String = StringContext(_sc.parts: _*) s (args: _*)
Copy link
Contributor

Choose a reason for hiding this comment

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

StringContext(_sc.parts: _*) can be replaced by _sc, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah... you're right, nice one.

@TomasMikula
Copy link
Contributor

I would exercise the principle of least power here and go with this solution over the macro one. It is much more transparent to the casual reader what is going on.

@peterneyens
Copy link
Collaborator

This looks great 👍

That implicit conversion to Shown is a nice trick.

Would syntax.show be a better place ? So you can get it using import cats.syntax.show._ (or cats.implicits._) instead of import cats.Show._ ?

@dwijnand
Copy link
Contributor Author

dwijnand commented Dec 12, 2016 •
edited
Loading

Probably. Or its own syntax object. Happy to go either way.

@travisbrown
Copy link
Contributor

👍 to supporting both cats.syntax.show._ and cats.implicits._ if we go this route (which I'm still not sure I prefer over #1489).

@adelbertc
Copy link
Contributor

👍

@dwijnand
Copy link
Contributor Author

cats.syntax.show._, cats.syntax.all._ and cats.implicits._ are all supported now.

@dwijnand
Copy link
Contributor Author

Looking for another maintainer to 👍 this.

@kailuowang
Copy link
Contributor

👍

@kailuowang kailuowang merged commit e233573 into typelevel:master Dec 21, 2016
@dwijnand dwijnand deleted the show-interp branch December 21, 2016 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomasMikula TomasMikula TomasMikula left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dwijnand @codecov-io @TomasMikula @peterneyens @travisbrown @adelbertc @kailuowang