scala / scala-collection-compat

makes some Scala 2.13 APIs (primarily collections, also some others) available on 2.11 and 2.12, to aid cross-building
Apache License 2.0
203 stars 87 forks source link

Add `.toXOption` methods from `StringOps` #554

Closed jxnu-liguobin closed 1 year ago

jxnu-liguobin commented 2 years ago

There are some useful method in scala2.13.x, such as toLongOption, But it throws an NPE when the string is null, so probably not many people use it?

SethTisue commented 2 years ago

The usual style in Scala is to assume that things aren't null. So I doubt that many people are avoiding toLongOption and friends for that reason. If a String is null, throwing seems expected to me. The idiomatic way to call toLongOption on a String which the programmer suspects might be null, probably because it came from some Java code, is Option(myStringOrNull).flatMap(_.toLongOption).

(I feel like there was a pretty thorough discussion in scala/bug about nulls and extension methods, but I can't seem to find it...)

In any case, I do think that toLongOption and friends would be welcome additions to this library.

jxnu-liguobin commented 2 years ago

The usual style in Scala is to assume that things aren't null. So I doubt that many people are avoiding toLongOption and friends for that reason. If a String is null, throwing seems expected to me. The idiomatic way to call toLongOption on a String which the programmer suspects might be null, probably because it came from some Java code, is Option(myStringOrNull).flatMap(_.toLongOption).

Because I find many people like using Try(catch NPE). 😸

SethTisue commented 2 years ago

@rjolly and I are looking at this at the Open Source Spree in Paris.

We're starting with just toLongOption as an example and we can worry later about whether anything needs to change when we generalize to other methods.

One question here is where the new code should live. compat/src/main/scala-2.12/scala/collection/compat/package.scala already exists and is a grab bag of different conversions. One could argue that the /collection/ part is inappropriate for String, but I looked in the Scala 2.13 standard library and all of StringOps, including non-collection methods such as toLongOption, is in scala.collection, so I think we could follow that precedent and put our code for this issue in the scala.collection.compat package.

SethTisue commented 2 years ago

I think the steps to a solution here are:

SethTisue commented 2 years ago

Note that I've narrowed the scope of the ticket to just the .toXOption methods. If there are are other extension methods on String in 2.13 that are desired, please open separate tickets on them, to make it easier for contributors to make bite-sized contributions.

SethTisue commented 2 years ago

@jxnu-liguobin about null handling, I see now that Martijn gave a design justification at https://github.com/scala/scala/pull/6538#discussion_r182983877

martijnhoekstra commented 2 years ago

My own justification there is very thin: It chooses to throw consistently throw a NPE over a NumberFormatException, with an at-least-better-than-its-now justification.

The probably almost zero cost of the null check (the benchmarks in the original MR could be used to verify that assumption of near zero cost) means to me that added convenience can be pretty small to be worth it too.

In this specific case, because these things are explicitly for validation, I can get behind the idea of doing the null check, even if it doesn't occur in idiomatic scala. The whole point is to do quick validation, and Option(str).flatMap(_.toXoption) might slow down the happy path by more than reasonable.

I thought it was only called an Open Source Spree if it was held around the Spree region in Berlin, and that in Paris it's just a sparkling source Seine.