Skip to content
This repository was archived by the owner on Oct 14, 2025. It is now read-only.

Only listen to stdin if --stdin flag is given#1003

Merged
es128 merged 1 commit intobrunch:masterfrom
josevalim:jv-stdin
Aug 5, 2015
Merged

Only listen to stdin if --stdin flag is given#1003
es128 merged 1 commit intobrunch:masterfrom
josevalim:jv-stdin

Conversation

@josevalim
Copy link
Copy Markdown
Contributor

Closes #998

I would really appreciate if there was a release so we can update Phoenix to use the --stdin flag from now on.

Also, I did not make the flag hidden, because folks would see it and wonder what it is about. I tried to make it as descriptive as possible though.

Thank you!

src/cli.coffee Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about the idea of the shorthand being just - to follow the convention of other unix tools?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@es128 - It seems like the - option works fine, but then I saw this elsewhere:

10:51 AM shreeve@ice ~/Data/Work/Code/xo-next> brunch watch -s -
Error: ENOENT, no such file or directory
  at Error (native)
  at process.chdir (/Users/shreeve/Data/Work/Code/brunch/node_modules/chokidar/node_modules/readdirp/node_modules/graceful-fs/polyfills.js:18:9)
  at module.exports.watch (/Users/shreeve/Data/Work/Code/brunch/src/watch.coffee:587:15)
  at start (/Users/shreeve/Data/Work/Code/brunch/src/index.coffee:9:3)
  at Command.listener (/Users/shreeve/Data/Work/Code/brunch/node_modules/commander/index.js:289:8)
  at Command.emit (events.js:110:17)
  ...

Is this easy to cleanup? If so, then the - option seems preferable to -l.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes that's probably easy to clean up at https://github.com/brunch/brunch/blob/master/src/watch.coffee#L585-L586

Either just wrap in a try or specifically exclude -

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess commander thinks the - option is just a leftover argument, so it gets treated differently and does not act the same as passing --stdin. Perhaps using the -l short option would work best. Unless you can poach the - off and make it act like --stdin.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided to not use "-" because it usually means you can write stuff in
the stdin and it will be read. Here we just care about EOF.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh ok, I had taken your word for it before that it would work. So I suppose we should just leave it like this, or just remove the shorthand flag.

@josevalim why did you choose -l?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I choose -l for "listen" but feel free to remove it. I am not attached to
it. :)

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's your PR 😄

I don't have strong feelings about it, but tempted to say I'd prefer leaving it at just the explicit --stdin. It's not going to be manually typed in enough (or ever) to justify a shorthand letter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+ to es128

@josevalim
Copy link
Copy Markdown
Contributor Author

Force pushed. This is ready to go!

es128 added a commit that referenced this pull request Aug 5, 2015
Only listen to stdin if --stdin flag is given
@es128 es128 merged commit ee4839d into brunch:master Aug 5, 2015
@josevalim
Copy link
Copy Markdown
Contributor Author

Please do a release when you can so I can update Phoenix. :)

@es128
Copy link
Copy Markdown
Member

es128 commented Aug 5, 2015

b8b25bc

@josevalim
Copy link
Copy Markdown
Contributor Author

Thank you! I appreciate it! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants