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 the sm"" interpolator to the standard library. #6254

Closed
wants to merge 2 commits into from

Conversation

hrhino
Copy link
Contributor

@hrhino hrhino commented Jan 9, 2018

This has been a feature available to the compiler since a0001fc (back in 2012!), but somehow never made it to the masses. It's useful, so let's put it in StringContext so all can enjoy its margin-stripping goodness.

Rather than just copy the implementation over, I've taken the liberty of turning it into a macro. Since there are plans to intrinsify the s"" and raw"" interpolators in the near future, the macro emits a call to raw"" in the form that will be expected by that transformation. Once it's merged, I'll add a test that the macro does indeed trigger the transformation.

Since the new interpolator can't yet be used in reflect/compiler, there's a private[scala] shim sm0 for their benefit. After reSTARR, I'll change them back to the stdlib one.

I published this locally and used it as a STARR with the compiler/reflect usages restored to sm""; everything seems to work fine. I've pushed up a dummy commit that does that so Jenkins can double-check.

  • remove [nomerge] commit before merge

This has been a feature available to the compiler since
a0001fc (back in 2012!), but somehow never made it to
the masses. It's useful, so let's put it in `StringContext`
so all can enjoy its margin-stripping goodness.

Rather than just copy the implementation over, I've taken
the liberty of turning it into a macro. Since there are
plans to intrinsify the `s""` and `raw""` interpolators
in the near future, the macro emits a call to `raw""` in
the form that will be expected by that transformation.

Since the new interpolator can't yet be used in reflect/compiler,
there's a `private[scala]` shim `sm0` for their benefit. After
reSTARR, I'll change them back to the stdlib one.
@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Jan 9, 2018
@hrhino hrhino requested review from retronym, lrytz and som-snytt and removed request for retronym January 9, 2018 22:54
@retronym
Copy link
Member

I don't think "sm" to conveys the meaning well enough for a standard interpolator. We sort of Huffman code "s" and "f" as they are very common, and then we have a longer name for "raw" which is a bit more niche. By that reasoning, stripMargin""" ... """ might be better here. Unfortunately, it doesn't give a clue as to whether escapes are processed or raw; one needs to read the documentation.

Another weakness of the interpolator as I wrote it is that it is hard coded to marginChar = '|'. It seems reasonable to expect that the interpolator form would have the same power as StringLike.stripMargin. But passing config to an interpolator requires some trickery (implicit param? implicit val addInterpolator: StringContext => { def stripMargin ... }?) that doesn't have precedent among the existing interpolators.

So this needs to be weighed up against the benefit from the problem which this interpolator solves (arguably surprising results when interpolated values contain newlines and the margin char on the next line, but didn't want it stripped).

@hrhino
Copy link
Contributor Author

hrhino commented Jan 10, 2018

I don't think "sm" to conveys the meaning well enough for a standard interpolator.

That's probably fair. stripMargin feels a bit long, but I'm not sure I could come up with a shorter one. The bikeshed will look lovely either way.

Another weakness of the interpolator as I wrote it is that it is hard coded to marginChar = '|'.

I wonder how many people use the one-arg form of .stripMargin. I don't think it's a good idea at all to allow that sort of "magical" customization; using implicit parameters for configuration might be a pretty common pattern but it's unobvious enough that I have doubts about using it here. Moreover, this shouldn't replace .stripMargin; if people need a different margin character it's not that onerous to call .stripMargin('✂') if that's how you want it.

I happen to like having this around, and I've put it in my own code at times. It was suggested that it might be a reasonable stdlib addition. If the detriments prove too detrimental, then so be it.

@som-snytt
Copy link
Contributor

I don't agree with retronym's comment that it must be maximally flexible in order to be maximally useful.

Picking a useful interpolator name is hard. It just occurred to me this second that a suffix operator could do the stripping at compile time, s"some $string\n\t|with lines".stripped. The machinery for that couldn't be absurd.

@propensive
Copy link
Contributor

It would be quite easy with Contextual, but that's a dependency that doesn't want to be in the standard library...

There's a difference between stripping at compile-time or runtime, of course. Aside from the better performance, it might actually be more desirable not to strip margins in substituted text (that's not known at compile time).

@propensive
Copy link
Contributor

As a suggestion for what to call it, how about crop""? My first choice would have been trim, but that's already been adorned with a more specific meaning in Java. crop has parallels with a similar operation on images.

@hrhino
Copy link
Contributor Author

hrhino commented Jan 11, 2018

I provoked a gitter discussion about this, which I'm archiving here in case anyone cares that their opinions not vanish down the gitter memory hole.

@retronym
Copy link
Member

it might actually be more desirable not to strip margins in substituted text

That was what originally prompted me to add sm as a utility to the compiler.

@SethTisue SethTisue added the WIP label Feb 24, 2018
@hrhino hrhino closed this Apr 23, 2018
@SethTisue SethTisue modified the milestones: 2.13.0-M4, 2.13.0-M5 May 4, 2018
@som-snytt
Copy link
Contributor

som-snytt commented Jul 21, 2018

cf #6957 following my previous comment.

I was going to follow @propensive 's suggestion of crop"" and title it, "Scala comes a cropper."

https://www.phrases.org.uk/meanings/come-a-cropper.html

@SethTisue SethTisue removed this from the 2.13.0-M5 milestone Aug 22, 2018
@som-snytt som-snytt removed their request for review April 5, 2023 15:51
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