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
SI-9656 Distinguish Numeric with step type #5175
Conversation
e44e1d4
to
38ea01e
Compare
val preposition = if (isInclusive) "to" else "until" | ||
f"$start $preposition $end${ | ||
if (step == 1) "" else f" by $step" | ||
}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to use f
instead of s
? f
will pull in a lot of code in Scala.js code bases that do not otherwise use String.format
.
Also, I'm not convinced the nested interpolator is more readable than:
s"$start $preposition $end" + (if (step == 1) "" else s" by $step")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a finger twitch. At first I wrote f"$start%d"
, I think. But it's interesting to hear the JS consequences. f-interpolator needs to special case if it's just appending strings. It already detects constant results.
For some reason, I really want the multiline interpolated expression to be readable, but that hasn't actually happened yet.
ps) created an issue for f"". https://issues.scala-lang.org/browse/SI-9784
38ea01e
to
92e1500
Compare
Observes @sjrd observations, and adds whether a range is empty or inexact. Squashed to recognize @stevorobs3 initiative. |
Also fixes the bogus |
/rebuild |
Thanks @adriaanm . I just sent the error to my colleague who was dealing with groovy exceptions in gradle last week. I said, "It's easy to make fun of things when they don't work, so that's what I'll do." |
Pleasure! This time we squirreled away too many snapshots. You never know when they might come in handy, I guess. |
I like this version. It gives a nice readable output like #5001 while preserving the distinction between |
I'm a bit concerned as this change makes Wouldn't it be possible to wrap the output in |
How about |
92e1500
to
1dfd094
Compare
|
The idea was just to distinguish what the step is.
This could be nicer:
|
Actually, I'm not convinced by "step of". It would be better if the Also, strings are typically ambiguous:
|
So I didn't even realize that you don't normally get precise stepping goodness. Possibly the comments about it not being complete mean you must opt in:
Maybe this is an improvement:
|
For Range and NumericRange, toString will indicate the step if it is not 1. Additionally, indicate empty ranges and ranges which are not "exact". For a "mapped" range, used by `Range.Double`, toString includes the underlying range and the simple type of the step (to distinguish Double from BigDecimal).
1dfd094
to
214ea82
Compare
Edited the first comment. |
Marking as on-hold for now in case you want to make further changes. I think the current state is acceptable but the proposed changes would also be fine with me. |
I believe I'm done. As in done done. |
LGTM |
Shorthand
toString
forRange
andNumericRange
.No parens because
Range(0 to 1)
looks like a factory but just gives a confusing error message.