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

Refactor to ES6 #10

Merged
merged 5 commits into from
Mar 10, 2016
Merged

Refactor to ES6 #10

merged 5 commits into from
Mar 10, 2016

Conversation

levithomason
Copy link
Contributor

This PR:

  1. refactors /src/parseArgs.js to ES6 syntax
  2. extends airbnb (instead of legacy)

@levithomason levithomason mentioned this pull request Mar 10, 2016
@levithomason
Copy link
Contributor Author

This is rebased and ready to go. Note, I replicated the existing functionality of parseArgs but I'm not sure it is right to start with. It is setup to accept an array of args like so:

['foo', 'bar;baz;quz', 'qux']

But, you can't actually get that array from process.argv. If you added ; in your command then your shell is going to split the command up. You'll need to quote it:

node cli.js "foo bar;baz;quz qux"

[..., ..., 'foo bar;baz;quz qux']

In which case you get that ^ as a string. So, i think we should reconsider how args are parsed, in another PR. Might be a good idea to consider something like yargs as well, depending on the needs.

@levithomason
Copy link
Contributor Author

@nfischer cc

}
// ['foo; bar', 'baz;quz', 'qux'] => [['foo'], ['bar', 'baz'], ['quz', 'qux']]
const parseArgs = (args) => args
.join(' ') // ['foo; bar baz;quz qux']
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work.

Imagine I invoke shx with something like this:

shx rm -rf "dir with spaces"

This should result in shell.rm('-rf', 'dir with spaces');, but with this it will become shell.rm('-rf', 'dir', 'with', 'spaces');

@ariporad
Copy link
Contributor

@levithomason: You could get that by doing spawn('/path/to/shx', 'foo', 'bar baz;quz');.

@levithomason
Copy link
Contributor Author

Right you are. I'd like to pause here for tests actually. I'd be much more comfortable with test cases for these. How about you?

@ariporad
Copy link
Contributor

Me too... I have some, let me push to master

@ariporad
Copy link
Contributor

Dammit... I don't know where the tests went, I suppose the got deleted. I'll try to re-write them tonight.

@levithomason
Copy link
Contributor Author

Cool, I'm also testing some approaches here. let's compare soon.

What is the use case for the semicolon?

@levithomason
Copy link
Contributor Author

Reason I ask, using a semicolon requires quoting the command to prevent the shell running shx from splitting the command:

$ shx "touch foo; rm foo"

It doesn't seem like that intuitive or useful of a feature, imo. If not for the semicolon parsing, I believe argv.slice(2) can be passed directly to shx with no transform. Consider these commands and their argv output;

shx rm dir
//=> ['rm', 'dir']

shx rm dir "with spaces"
//=> ['rm', 'dir with spaces']

shx rm -r "dir with spaces"
//=> ['rm', '-r', 'dir with spaces']

shx rm "dir with spaces" "multi dir"
//=> ['rm', 'dir with spaces', 'multi dir']

shx rm -rf "dir with spaces" "multi dir"
//=> ['rm', '-rf', 'dir with spaces', 'multi dir']

Is there a semicolon use case that is a big enough win to have an arg parser?

@nfischer
Copy link
Member

Assuming we want to create and remove a file:

# I like this
$ shx touch file.txt ; shx rm file.txt
# I don't like this
$ shx "touch file.txt;rm file.txt"

Why over complicate? I'm assuming semicolon has the same behavior on Windows, of course.

@ariporad
Copy link
Contributor

Hmm... That's a good point. The semicolon would get caught by the Shell. Maybe
we should scrap that. On Wed, Mar 9, 2016 at 5:39 PM, Nate Fischer [email protected] wrote:
Assuming we want to create and remove a file:

I like this

$ shx touch file.txt ; shx rm file.txt

I don't like this

$ shx "touch file.txt;rm file.txt"

Why over complicate? I'm assuming semicolon has the same behavior on Windows, of
course.


Reply to this email directly or view it on GitHub
[https://github.com//pull/10#issuecomment-194610409] .[https://github.com/notifications/beacon/ABu7pP32ZG27YVHpyGIEVgz7MmvuM0n3ks5pr3ZjgaJpZM4HsebW.gif]

@levithomason
Copy link
Contributor Author

Cool beans, totally agreed. Since shx is a command, each command separated by a semi should start with it. Just as if you were chaining git commands. If we do end up with some kind of repl like in #12, then I could see an arg parser in that context.

I'll update this PR to use the simplified method then and we'll truck on! Thanks.

@levithomason levithomason changed the title ES6 parseArgs Refactor to ES6 Mar 10, 2016
@@ -1,3 +1,5 @@
#!/usr/bin/env node
var shx = require('./shx');
import 'babel-polyfill';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Polyfill needs to be included in the entry point. We don't yet have bundles so I am treating the cli as the entry point. This lets us use future native features, like the includes method for arrays.

@levithomason
Copy link
Contributor Author

OK, we should be all set now. Dropped parseArgs and updated all to ES6.

@ariporad
Copy link
Contributor

@levithomason: could I ask you to squash your commits, and have them comply to the angular commit message conventions? Thanks so much!

@levithomason
Copy link
Contributor Author

Added support for future ret.code use. Reworked commits to use Angular's guideline. I've been meaning to pick it up for a while now, thanks for the push!

It didn't seem like all those should be squashed, but LMK if you disagree and I'll squash 'em.

@nfischer
Copy link
Member

I don't care as much about squashing commits. They seem unrelated enough that we can keep them as-is.

@ariporad
Copy link
Contributor

Agreed.

@ariporad
Copy link
Contributor

LGTM!

ariporad added a commit that referenced this pull request Mar 10, 2016
@ariporad ariporad merged commit 4923012 into shelljs:master Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants