Use an Enum for process stdio redirections #4445

ysbaddaden · 2017-05-22T10:12:45Z

Implements a Process:Redirect enum as per #3084 to define the standard input, output and error IO of spawned processes. This patch replaces the inexplicit and confusing nil, true and false values for the explicitness of PIPE, CLOSE and INHERIT.

closes #3084

bew · 2017-05-22T10:23:56Z

src/process.cr

+                input : Stdio = INHERIT,
+                output : Stdio = INHERIT,
+                error : Stdio = INHERIT,
+                chdir : String? = nil)


I suggest to inline this method signatures like thoses above and below this one

input : Stdio = INHERIT, output : Stdio = INHERIT, error : Stdio = INHERIT, chdir : String? = nil)

bew · 2017-05-22T10:47:09Z

src/process.cr

+    INHERIT
+  end
+
+  PIPE    = Redirect::PIPE


I think we need documentation on thoses constants too

I guess, but looking at the core/stdlib we don't seem to document enums much, so I'm not sure what I should document here. The copied constants? The enum itself or each enum value?

I add a proposal for naming constant comments above.

I suppose that documentation can be copied, but we can make #See whatever too.

Or, may be, we can to private PIPE = Redirect::PIPE?

Yes, private constants are supported, so you'll can set aliases as private.

enum Whatever One Two end private WH = Whatever::One p WH

https://play.crystal-lang.org/#/r/227w

@akzhan using private will block you from accessing PIPE from outside Process namespace.

As per #3084 the aliases are to be used outside of the Process class, so we don't have to type the lengthy Process::Redirect::CLOSE but Process::CLOSE. I wish symbols were transformed automatically to enum values (when an enum is expected), so we could just use :close.

akzhan · 2017-05-22T17:14:23Z

src/process.cr

-  alias Stdio = Nil | Bool | IO
+  # How to redirect the standard input, output and error IO of a process.
+  enum Redirect
+    PIPE


# Use a pipe

akzhan · 2017-05-22T17:16:14Z

src/process.cr

+  # How to redirect the standard input, output and error IO of a process.
+  enum Redirect
+    PIPE
+    CLOSE


# Redirect to /dev/null

akzhan · 2017-05-22T17:17:15Z

src/process.cr

+  enum Redirect
+    PIPE
+    CLOSE
+    INHERIT


# Inherit from parent process

akzhan

Anyway we must use Process::Redirect::PIPE outside of the class, as I suppose.

No Process::PIPE, so PIPE alias should be private.

akzhan · 2017-05-22T17:24:50Z

src/process.cr

+    INHERIT
+  end
+
+  PIPE    = Redirect::PIPE


Yes, private constants are supported, so you'll can set aliases as private.

enum Whatever One Two end private WH = Whatever::One p WH

https://play.crystal-lang.org/#/r/227w

bew · 2017-05-23T13:12:49Z

@akzhan the advantage of defining thoses constants (Process::CLOSE, ...) is to not use the looong Process::Redirect::CLOSE but to have still thoses defined as an enum (and typed as the Redirect enum)..

The objective is to use them as Process::CLOSE

We could make the enum Redirect private, but it doesn't really make sense since we couldn't use Process::Redirect as type restriction outside..

ysbaddaden · 2017-05-23T13:18:59Z

I added a tiny explanation of enum values. If nobody objects, I'll merge this a bit later.

luislavena · 2017-05-23T13:20:51Z

Anyway we must use Process::Redirect::PIPE outside of the class, as I suppose.

That is why Process::PIPE is set for, but shouldn't be private so can be used:

module Foo
  enum Bar
    ONE = 1
    TWO = 2
  end
  
  ONE = Bar::ONE
  private TWO = Bar::TWO
end

p Foo::ONE # => 1

# uncomment the following fails to compile (Error in line 14: private constant Foo::TWO referenced)
# p Foo::TWO

bew

LGTM!

Is this considered as a breaking change for next release?

ysbaddaden · 2017-05-23T13:25:36Z

Yes. It's a breaking change. For the greater good, I hope.

luislavena

LGTM

akzhan · 2017-05-23T13:32:12Z

LGTM too.

bew · 2017-07-28T08:27:09Z

So is this ready for merge?

RX14 · 2017-07-28T09:56:11Z

src/process.cr

+  enum Redirect
+    # Pipe the IO so the parent process can read (or write) to the process IO
+    # throught `#input`, `#output` or `#error`.
+    PIPE


Why are the enum constants all caps? Usually enums use PascalCase.

My habit. They're constants, not types, so I always put them as uppercase, not camelcase. I don't mind changing this.

I think there are a lot of other places in the stdlib where the enum constants are pascal case, so this should be changed for consistency.

ysbaddaden · 2017-08-01T19:43:54Z

I renamed enum constant to camelcase.

RX14 · 2017-08-01T21:45:06Z

src/process.cr

+    Inherit
+  end
+
+  PIPE    = Redirect::Pipe


I would have renamed these to also be camel case to match the enum constants. You could argue either way but what does everyone else think?

Yeah, they're constants that happen to be an enum value. It would be confusing to start having camelcase constants —thought there are some camelcase constants (e.g. under Time) which irritates me.

Hence why I believe all constants, including enum constants, should always be uppercase, while types are always camelcase.

Simple rule, no confusion.

Yeah, where's that camelcase for enum values coming from anyway? :/

Even though they are technically constants, usage of Enum values is closer to symbols (in a sense that their only value is their name), so i prefer them to be camelcase, not SCREAMING on me. But it's a matter of taste of course.

Personally, I'd consider removing these constants, stdin: Process::Redirect::Pipe is more explanatory to me and looks nicer than stdin: Process::PIPE. I don't like the inconsistency in case between the enum constants and the constants on Process.

ysbaddaden · 2017-08-11T09:43:05Z

Changed to use the enum. I really wish symbols were translated to enum values at compile time, because typing Process::Redirect::X multiple times in a single method call really hits readability.

RX14 · 2017-08-11T17:05:06Z

Just as a quick question, does anyone actually use symbols for a usecase which couldn't reasonably be replaced by an enum? Perhaps it would be easier to remove symbols and use the syntax for the much more common enum shorthand?

ysbaddaden · 2017-08-11T21:44:08Z

Yup: named_tuple[:key], along others. There is no need to remove symbols, just to interpret them as case-insensitive enum values in semantic phase.

asterite

Great work!

See: crystal-lang/crystal#4445

bew reviewed May 22, 2017

View reviewed changes

ysbaddaden force-pushed the fix-process-run-stdio-argument branch from 056439c to e6ce3f4 Compare May 22, 2017 10:31

bew reviewed May 22, 2017

View reviewed changes

akzhan reviewed May 22, 2017

View reviewed changes

src/process.cr Outdated

enum Redirect

PIPE

CLOSE

INHERIT

Copy link

Contributor

akzhan May 22, 2017

Choose a reason for hiding this comment

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

# Inherit from parent process

akzhan reviewed May 23, 2017

View reviewed changes

ysbaddaden force-pushed the fix-process-run-stdio-argument branch from e6ce3f4 to 309f71a Compare May 23, 2017 13:17

bew approved these changes May 23, 2017

View reviewed changes

luislavena approved these changes May 23, 2017

View reviewed changes

RX14 reviewed Jul 28, 2017

View reviewed changes

ysbaddaden force-pushed the fix-process-run-stdio-argument branch from 309f71a to caacef2 Compare August 1, 2017 19:43

RX14 reviewed Aug 1, 2017

View reviewed changes

ysbaddaden force-pushed the fix-process-run-stdio-argument branch from caacef2 to 543607f Compare August 11, 2017 09:39

Use an Enum for Process stdio redirections

b45d6f0

ysbaddaden force-pushed the fix-process-run-stdio-argument branch from 543607f to b45d6f0 Compare August 11, 2017 21:49

asterite approved these changes Aug 19, 2017

View reviewed changes

RX14 approved these changes Aug 19, 2017

View reviewed changes

RX14 added kind:refactor topic:stdlib labels Aug 19, 2017

RX14 merged commit 4bec310 into crystal-lang:master Aug 19, 2017

RX14 added this to the Next milestone Aug 19, 2017

ysbaddaden deleted the fix-process-run-stdio-argument branch August 20, 2017 16:18

ysbaddaden mentioned this pull request Aug 20, 2017

Process compatibility on Crystal > 0.23.1 crystal-lang/shards#166

Closed

samueleaton mentioned this pull request Aug 26, 2017

Updates to make compatible with upcoming crystal update samueleaton/sentry#15

Merged

agibralter added a commit to agibralter/git-sleuth that referenced this pull request Jul 14, 2018

Fix Process.new for crystal breaking changes

2f7bfc0

See: crystal-lang/crystal#4445

Use an Enum for process stdio redirections #4445

Use an Enum for process stdio redirections #4445

Conversation

ysbaddaden commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden May 23, 2017 • edited Loading

Choose a reason for hiding this comment

akzhan May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented May 23, 2017 • edited Loading

ysbaddaden commented May 23, 2017 • edited Loading

luislavena commented May 23, 2017

bew left a comment

Choose a reason for hiding this comment

ysbaddaden commented May 23, 2017

luislavena left a comment

Choose a reason for hiding this comment

akzhan commented May 23, 2017

bew commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

ysbaddaden commented Aug 11, 2017

RX14 commented Aug 11, 2017 • edited Loading

ysbaddaden commented Aug 11, 2017 • edited Loading

asterite left a comment

Choose a reason for hiding this comment

akzhan May 22, 2017 •

edited

Loading

ysbaddaden May 23, 2017 •

edited

Loading

akzhan May 22, 2017 •

edited

Loading

akzhan left a comment •

edited

Loading

bew commented May 23, 2017 •

edited

Loading

ysbaddaden commented May 23, 2017 •

edited

Loading

RX14 Aug 2, 2017 •

edited

Loading

RX14 commented Aug 11, 2017 •

edited

Loading

ysbaddaden commented Aug 11, 2017 •

edited

Loading