Skip to content
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

added process.run example. #9485

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vectorselector
Copy link

I am not sure if this is correct way to contribute to project. (forked then edited, then PR'ed)
Added process.run example at line 143.
I am not sure if only demonstrating output is ok or if you want input and error as part of it also
I am also unsure if you are still using IO::Memory.new.

I am not sure if this is correct way to contribute to project. (forked then edited, then PR'ed)
Added process.run example at line 143.
I am not sure if only demonstrating output is ok or if you want input and error as part of it also
I am also unsure if you are still using IO::Memory.new.
src/process.cr Outdated Show resolved Hide resolved
@@ -126,6 +126,7 @@ class Process
# Executes a process and waits for it to complete.
#
# By default the process is configured without input, output or error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

# command = "echo #{someId}"
# output = IO::Memory.new
# status = Process.run(command, shell: true, output: output)
# pp output.to_s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not a good example, as this is better solved with Process.run(command) {|proc| pp proc.output.gets_to_end } or simply pp `#{command}`. Secondarily it demonstrates interpolating arguments without proper quoting over the preferred and safer way of just populating args. Finally I don't quite see the need for the command local variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a good idea to make this better would be using cat with input and output streams. The example would write to input and read the same content from output.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thank you, i'll try again

@straight-shoota
Copy link
Member

Thanks for this!

I am not sure if this is correct way to contribute to project. (forked then edited, then PR'ed)

That's exactly how this is supposed to work. You can find more information on this in CONTRIBUTING.md

I am not sure if only demonstrating output is ok or if you want input and error as part of it also

An error example wouldn't hurt, but it's also not necessary. Having a good example of working behaviour is most important.

I am also unsure if you are still using IO::Memory.new.

Yeah, that's correct.
You would often use String.build instead, which is more optimized for building strings from code. For this use case it doesn't make a difference, though, because the stream comes from a pipe.

@straight-shoota straight-shoota added kind:docs pr:needs-work A PR requires modifications by the author. topic:stdlib:system labels Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:docs pr:needs-work A PR requires modifications by the author. topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants