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

Use an Enum for process stdio redirections #4445

Merged
merged 1 commit into from
Aug 19, 2017

Conversation

ysbaddaden
Copy link
Contributor

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

src/process.cr Outdated
input : Stdio = INHERIT,
output : Stdio = INHERIT,
error : Stdio = INHERIT,
chdir : String? = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@ysbaddaden ysbaddaden force-pushed the fix-process-run-stdio-argument branch from 056439c to e6ce3f4 Compare May 22, 2017 10:31
src/process.cr Outdated
INHERIT
end

PIPE = Redirect::PIPE
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need documentation on thoses constants too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@akzhan akzhan May 22, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ysbaddaden ysbaddaden May 23, 2017

Choose a reason for hiding this comment

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

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.

src/process.cr Outdated
alias Stdio = Nil | Bool | IO
# How to redirect the standard input, output and error IO of a process.
enum Redirect
PIPE
Copy link
Contributor

@akzhan akzhan May 22, 2017

Choose a reason for hiding this comment

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

# Use a pipe

src/process.cr Outdated
# How to redirect the standard input, output and error IO of a process.
enum Redirect
PIPE
CLOSE
Copy link
Contributor

Choose a reason for hiding this comment

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

# Redirect to /dev/null

src/process.cr Outdated
enum Redirect
PIPE
CLOSE
INHERIT
Copy link
Contributor

Choose a reason for hiding this comment

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

# Inherit from parent process

Copy link
Contributor

@akzhan akzhan left a comment

Choose a reason for hiding this comment

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

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

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

src/process.cr Outdated
INHERIT
end

PIPE = Redirect::PIPE
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

bew commented May 23, 2017

@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 ysbaddaden force-pushed the fix-process-run-stdio-argument branch from e6ce3f4 to 309f71a Compare May 23, 2017 13:17
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented May 23, 2017

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

@luislavena
Copy link
Contributor

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

Copy link
Contributor

@bew bew left a comment

Choose a reason for hiding this comment

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

LGTM!

Is this considered as a breaking change for next release?

@ysbaddaden
Copy link
Contributor Author

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

Copy link
Contributor

@luislavena luislavena left a comment

Choose a reason for hiding this comment

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

LGTM

@akzhan
Copy link
Contributor

akzhan commented May 23, 2017

LGTM too.

@bew
Copy link
Contributor

bew commented Jul 28, 2017

So is this ready for merge?

src/process.cr Outdated
enum Redirect
# Pipe the IO so the parent process can read (or write) to the process IO
# throught `#input`, `#output` or `#error`.
PIPE
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ysbaddaden force-pushed the fix-process-run-stdio-argument branch from 309f71a to caacef2 Compare August 1, 2017 19:43
@ysbaddaden
Copy link
Contributor Author

I renamed enum constant to camelcase.

src/process.cr Outdated
Inherit
end

PIPE = Redirect::Pipe
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple rule, no confusion.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@RX14 RX14 Aug 2, 2017

Choose a reason for hiding this comment

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

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 ysbaddaden force-pushed the fix-process-run-stdio-argument branch from caacef2 to 543607f Compare August 11, 2017 09:39
@ysbaddaden
Copy link
Contributor Author

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
Copy link
Contributor

RX14 commented Aug 11, 2017

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
Copy link
Contributor Author

ysbaddaden commented Aug 11, 2017

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.

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

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Great work!

@RX14 RX14 merged commit 4bec310 into crystal-lang:master Aug 19, 2017
@RX14 RX14 added this to the Next milestone Aug 19, 2017
@ysbaddaden ysbaddaden deleted the fix-process-run-stdio-argument branch August 20, 2017 16:18
agibralter added a commit to agibralter/git-sleuth that referenced this pull request Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing values for Process.run input, output and error
8 participants