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

Deprecate nonsensical StringOps operations #6702

Merged
merged 1 commit into from Jun 1, 2018

Conversation

Projects
None yet
5 participants
@szeiger
Copy link
Contributor

szeiger commented May 30, 2018

Fixes scala/bug#10881

This goes further than just the methods that I identified in #6565 (comment) by deprecating all methods that forward to WrappedString. I did not implement optimized versions in StringOps originally because these methods do not really make sense for String. Users can always call the WrappedString equivalents directly (if that's what they really want).

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone May 30, 2018

@@ -1287,9 +1288,11 @@ final class StringOps(private val s: String) extends AnyVal {
* ''n'' times in `that`, then the first ''n'' occurrences of `x` will be retained
* in the result, but any following occurrences will be omitted.
*/
@deprecated("Use `new WrappedString(s).intersect(...).self` instead of `s.diff(...)`", "2.13.0")

This comment has been minimized.

@hrhino

hrhino May 30, 2018

Member

"...instead of s.intersect(...)"

@@ -1299,6 +1302,7 @@ final class StringOps(private val s: String) extends AnyVal {
* @tparam B the type of the elements after being transformed by `f`
* @return a new string consisting of all the chars of this string without duplicates.
*/
@deprecated("Use `new WrappedString(s).distictBy(...).self` instead of `s.distictBy(...)`", "2.13.0")

This comment has been minimized.

@hrhino

hrhino May 30, 2018

Member

distict -> distinct

def combinations(n: Int): Iterator[String] = new WrappedString(s).combinations(n).map(_.self)

/** Iterates over distinct permutations.
*
* @return An Iterator which traverses the distinct permutations of this string.
* @example `"abb".permutations = Iterator(abb, bab, bba)`
*/
@deprecated("Use `new WrappedString(s).permutations(...).map(_.self)` instead of `s.combinations(...)`", "2.13.0")

This comment has been minimized.

@hrhino

hrhino May 30, 2018

Member

combinations -> permutations

@szeiger

This comment has been minimized.

Copy link
Contributor

szeiger commented May 30, 2018

@hrhino The kind of bugs that creep in when the workday gets too long :-)

@szeiger szeiger force-pushed the szeiger:issue/10881 branch from 2642030 to 7a33635 May 30, 2018

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented May 30, 2018

because these methods do not really make sense for String

Maybe some are useful / convenient to keep? In decreasing order of potential usefulness:

  • sliding (?)
  • distinct / distinctBy (??)
  • sorted / sortWith / sortBy (???)
@szeiger

This comment has been minimized.

Copy link
Contributor

szeiger commented Jun 1, 2018

You can still use all of them on a Seq[Char]. IMHO this conveys better that the individual Char values are meaningful (i.e. the sequence is not an arbitrary Unicode String). The methods are ill-defined for arbitrary Strings and can result in invalid char sequences.

@lrytz

lrytz approved these changes Jun 1, 2018

Copy link
Member

lrytz left a comment

Good point about unicode.

@lrytz lrytz merged commit 21eb884 into scala:2.13.x Jun 1, 2018

3 checks passed

cla @szeiger signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [2021] SUCCESS. Took 35 min.
Details

@szeiger szeiger deleted the szeiger:issue/10881 branch Jun 1, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 18, 2018

the deprecation messages say e.g.

       warning: method sorted in class StringOps is deprecated (since 2.13.0):
         Use `new WrappedString(s).sorted.unwrap` instead of `s.sorted`

but s.toSeq.sorted.toString seems more idiomatic to me

EDIT: better .mkString

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 20, 2018

mkString is slow, it's defined as @inline final def mkString: String = mkString(""), so it goes through a StringBuilder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment