Conversation
straight-shoota
changed the title
|
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/rfc-0025-modern-process-spawn-api/8773/1 |
| - Deprecation of legacy API methods. | ||
| - Deprecation of command literals. | ||
|
|
||
| ## Future possibilities |
There was a problem hiding this comment.
thought: I'm wondering about the details of handling newlines. Maybe this
Naturally, I'm not very inclined to the idea of altering the captured string. But I'm willing to give it a minute to think about in the interest of convenience.
Line breaks are different on different operating systems (simplified: Unix LF vs Windows CRLF).
In the prototype application of Process.capture for shards (crystal-lang/shards#701), this is a serious concern in particular for portability of specs.
I ended up normalizing line endings explicitly in a project-local helper method:
This is not an ideal solution though, considering that many other libraries face similar issues for expectations about the output from other processes.
I'm sure it would not be wise to implicitly normalize line breaks. But maybe there could be an explicit normalizer? Process::Result#normalize perhaps?
And/or since this is especially important for specs, maybe we could provide a spec helper for this?
To be fair: Shards specs currently have 3 examples with line breaks in the expected text. Two of them only test a single line, so there is currently only one multi-line expectation.
This is not huge. But I figure it might be useful to have more of those expectations. And for that we need an easy solution to make it convenient to use.
There was a problem hiding this comment.
The single multiline case in Shards can be rewritten to test all 3 lines independently, then we won't need to normalize anymore.
Now, that doesn't invalidate such a method, but #normalize might be too generic—what is it normalizing?
| - Deprecation of legacy API methods. | ||
| - Deprecation of command literals. | ||
|
|
||
| ## Future possibilities |
There was a problem hiding this comment.
thought: Another thought about newlines: When capturing output that consists of a single word (or similar) you typically want to remove a trailing newline.
So captured process output is quite often transformed with .chomp.
Maybe it's fine to have this explicit call.
But I'm also thinking maybe it could be more convenient to not require another call?
Would it be feasible for .capture to chomp implicitly? Maybe only when the output is a single line (i.e. index('\n') == size -1)?
As mentioned above, I'm very sceptical of altering the captured string. But maybe there is something in it?
There was a problem hiding this comment.
That should never be implicit: we know nothing about the output. It might not even be plain text but binary.
There was a problem hiding this comment.
I'd suggest adding a chomp parameter.
There was a problem hiding this comment.
I don't think a parameter would have much benefit over explicitly calling chomp on the result.
Perhaps we could potentially avoid the intermediary string allocation. But that doesn't work out of the box and would require support from IO#gets_to_end (or a custom implementation).
There was a problem hiding this comment.
True, although I thought it would make sense as there's already gets(chomp = true).
|
I added splat parameter overloads for all new |
| ```cr | ||
| # splat parameter | ||
| Process.run("crystal", "tool", "format", path) | ||
| Process.run("crystal", "tool", "format", *paths) # `paths` must be a Tuple |
There was a problem hiding this comment.
aside: Looking at this, I'm wondering if we could make splat parameters work with any collection type. It would mean the arguments cannot be represented as a tuple. It would require a dynamically sized container type. 🤔
The use case is not super strong of course, but it might be a bit of a nuisance.
Implement `Process.capture`, `Process.capture?` and `Process.capture_result` from [RFC 0025](crystal-lang/rfcs#25). This implements the basic capture behaviour, which may be enhanced in the future. In particular, this patch misses truncation of the captured error stream which will be a follow-up (#16774).
4 participants
Preview: https://github.com/crystal-lang/rfcs/blob/rfc/process-spawn-api/text/0025-process-spawn-api.md