scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Deprecate scala.sys.process.ProcessBuilder symbolic methods #11133

Open joshlemer opened 5 years ago

joshlemer commented 5 years ago

This was originally reported by @smarter

SethTisue commented 5 years ago

perhaps looking at Ammonite's design for their comparable API would be helpful

SethTisue commented 5 years ago

I'm unsure about the value and timing of these changes.

In general, scala.sys.process hasn't been touched much in recent years. The plan is to split it out into a separate module, but that didn't happen in time for 2.13.

I'm not sure it's desirable to change the API around so drastically before it becomes a separate, community-maintained module. I think it might be more desirable to leave the API alone in the meantime, for stability, especially this late in the 2.13 cycle.

Normally making a module proceeds first by modularizing it and doing a release without changing much, then once a backwards-compatible 1.0 release has been published and the independence of the module is established, consider changing things around for 1.1 or 2.0 or ...

I'm also not sure there would be a consensus — either before or after the move to a module — on deprecating the symbolic methods. (#> is a special case there; but by itself, the Dotty issue doesn't justify deprecating all of the symbolic methods at once.)

I perhaps shouldn't have slapped "help wanted" on this without being clear that I think some discussion is needed.

joshlemer commented 5 years ago

@SethTisue Maybe we should soften the requirements of this ticket to only ask for an addition of symbolic aliases.

smarter commented 5 years ago

Whether or not the symbolic methods should be deprecated, I think that adding non-symbolic methods is a great improvement that I'd love to see in 2.13.

lrytz commented 5 years ago

Would it even be necessary to remove method #>? Scala has separate namespaces for terms and types.

smarter commented 5 years ago

No, it's not necessary, it just makes things less confusing.

lrytz commented 5 years ago

The I agree with Seth to leave this as-is for now.

SethTisue commented 5 years ago

I've closed the PR for now, we could revisit if the #> stuff in Dotty moves forward

SethTisue commented 5 years ago

in other sys.process-related news, https://twitter.com/li_haoyi/status/1059650646616363008

lihaoyi commented 5 years ago

FWIW, Ammonite's filesystem/subprocess module has been extracted into a standalone, less-idiosyncratic library OS-Lib. OS-Lib's subprocess handling is heavily inspired by the Python subprocess API, and is both reasonably concise and operator-free:

// Start a long-lived python process which you can communicate with
val sub = os.proc("python", "-u", "-c", "while True: print(eval(raw_input()))")
            .spawn(cwd = wd)

// Sending some text to the subprocess
sub.stdin.write("1 + 2")
sub.stdin.writeLine("+ 4")
sub.stdin.flush()
sub.stdout.readLine() ==> "7"

// Sending some bytes to the subprocess
sub.stdin.write("1 * 2".getBytes)
sub.stdin.write("* 4\n".getBytes)
sub.stdin.flush()
sub.stdout.read() ==> '8'.toByte

sub.destroy()

// You can chain multiple subprocess' stdin/stdout together
val curl = os.proc("curl", "-L" , "https://git.io/fpfTs").spawn(stderr = os.Inherit)
val gzip = os.proc("gzip", "-n").spawn(stdin = curl.stdout)
val sha = os.proc("shasum", "-a", "256").spawn(stdin = gzip.stdout)
sha.stdout.trim ==> "acc142175fa520a1cb2be5b97cbbe9bea092e8bba3fe2e95afa645615908229e  -"

scala.sys.process hasn't been touched in more than a few years: the last significant changes were probably by mark harrah about a decade ago when it was still part of SBT. Apart from some trivial reformatting there hasn't been any real changes since Paul imported the whole thing from SBT into the scala/scala 8 (!) years ago. Honestly, at this point I don't think it's worth spending much effort trying to fix the API, and instead just focus on getting it out of scala/scala and start pointing people at better alternatives like the one above

som-snytt commented 4 years ago

I think what people meant was to add more symbolic operators.

I agree it's better to let sleeping dogs dream of running free in a meadow of flowers right after a rain storm. With lots of smells and mud.

I would take issue with the notion that the package hasn't been touched, as I've lost at least two weekends over several years to a few fixes, or sometimes breakages, so it's a non-trivial maintenance burden. Oh, in fact, I got a fix in the new point release, but that was only motivated by someone else's probably misguided enthusiasm.

It's too bad the package isn't already spun off, or spun out, for anyone to tinker with if they feel the need.

I was led over to this ticket because someone was talking about symbolic operators vs namely names again, and I think this is a good example of how the aliasing experiment has shown that the two don't mix, for reasons of precedence and general confusion.

So I would recommend against aliasing, and against kicking the dog.

I hope Seth closes this ticket and makes the link to Haoyi's stuff bigger in his announcement that sys.process will be decommissioned by sending it off to a "module". It's old cats who wander off to disappear when it's time; dogs need assistance.

eed3si9n commented 4 years ago

sys.process started as sbt.Process in 2009 and was brought into Scala 2.9.0 (https://github.com/scala/scala/commit/5bada810b4c7eda186aa40b94a78326520b3fa92) in 2011. I don't know the historical context of why this happened. In any case, sbt.Process was alive until sbt 1. If scala-library doesn't want it, I'd be happy to resurrect it on sbt/sbt and update the method names etc there.