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

Add argv parsing example #233

Merged
merged 3 commits into from
Mar 27, 2018
Merged

Add argv parsing example #233

merged 3 commits into from
Mar 27, 2018

Conversation

ivan-kleshnin
Copy link
Contributor

@ivan-kleshnin ivan-kleshnin commented Mar 25, 2018

Hey guys, can I suggest an example of argv (CLI) parsing?

1) NodeJS provides process.argv obviously but it's quite low-level. For example, I'd like to have

$ node argv.js foo -f bar

parsed as

["foo", ["f", true], "bar"]

instead of

["foo", "-f", "bar"]

You may also want to parse argv-like strings in Browser (to highlight code or whatever).

2) Moreover, this example showcases a parsing of quoted strings:

string: function() {
  // one of possible quotes, then sequence of anything except that quote (unless escaped), then the same quote
  return P.oneOf(`"'`).chain(function(q) {
    return P.alt(
      P.regex(new RegExp(`[^\\\\${q}]+`)),
      P.string(`\\`).then(P.any)
    )
      .many()
      .tie()
      .skip(P.string(q));
  });
}

with escape support, which no other example currently does (except json.js but not 100% close).

Any remarks are welcome. I followed the style of other examples – it passes $ npm run lint.

@coveralls
Copy link

coveralls commented Mar 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 715c5a7 on ivan-kleshnin:master into 13359d7 on jneen:master.

Copy link
Collaborator

@wavebeem wavebeem left a comment

Choose a reason for hiding this comment

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

This is a pretty cool idea. I literally never thought to use Parsimmon for this.

If you don't mind addressing these comments I'd be happy to merge this in :)

examples/argv.js Outdated
}
});

let args = process.argv.slice(2).join(" ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might make more sense to take the entire options string as argv[2] like node cli.js "--foo --qux=guz -b" to show that this is parsing out one whole string and not relying on the shell (bash, etc) to parse for us.

examples/argv.js Outdated
// one of possible quotes, then sequence of anything except that quote (unless escaped), then the same quote
return P.oneOf(`"'`).chain(function(q) {
return P.alt(
P.regex(new RegExp(`[^\\\\${q}]+`)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this regexp is pretty confusing to me. i think you might be able to rewrite this using some of the other parsimmon constructors?

you could also just write two separate string rules and then use alt() to combine them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, double escaping is going on here, not the cleanest code in the history :)
I agree with both points – will try to rewrite this as you suggested.

@ivan-kleshnin
Copy link
Contributor Author

ivan-kleshnin commented Mar 26, 2018

Can't find out, why Linter complains on this:

option: function(r) {
  return P.seq(
    P.alt(
      P.string("-").then(P.regex(/[a-z]/)),
      P.string("--").then(r.word)
    ),
    P.alt(
      P.string("=").then(r.word),
      P.of(true)
    )
  );
},

and wants me to change it to less readable this:

option: function(r) {
  return P.seq(
    P.alt(P.string("-").then(P.regex(/[a-z]/)), P.string("--").then(r.word)),
    P.alt(P.string("=").then(r.word),P.of(true))
  );
},

To clarify

Direct run with $ eslint examples/argv.js shows no warnings but $ npm run lint crashes without any clue:

> parsimmon@1.7.1 lint /Users/ivankleshnin/Sandboxes/parsimmon-fork
> prettier --list-different '{examples,src,test}/**/*.js' webpack.config.js && eslint examples src test webpack.config.js

examples/argv.js
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! parsimmon@1.7.1 lint: `prettier --list-different '{examples,src,test}/**/*.js' webpack.config.js && eslint examples src test webpack.config.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the parsimmon@1.7.1 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/ivankleshnin/.npm/_logs/2018-03-26T09_19_57_051Z-debug.log

until I run $ npm run lint:fix. Any idea why this may happen?

@wavebeem
Copy link
Collaborator

The full command behind npm run lint:fix is prettier --list-different '{examples,src,test}/**/*.js' webpack.config.js && eslint examples src test webpack.config.js

The problem is Prettier wants to reformat those lines. If alt(...) took an array instead I think it would format them how you want.

I think if you use a.or(b) instead of P.alt(a, b) Prettier may format this more how you like.

It might also be worth checking out if a newer version of Prettier formats that nicer.

@ivan-kleshnin
Copy link
Contributor Author

ivan-kleshnin commented Mar 27, 2018

It might also be worth checking out if a newer version of Prettier formats that nicer.

I checked and unfortunately no. v1.11 improved one minor thing in comparison to 1.7 in this code but not that one.

If alt(...) took an array instead I think it would format them how you want.

Tried this:

alt(...[
  ___
])

and also no. It wants me to fold an array into a single line...

I personally never use such tools – the're always too stupid and unable to pick normal cases.
But, your project – your rules, I left the code layout as this thing wants.

@wavebeem wavebeem merged commit f1d2dee into jneen:master Mar 27, 2018
@wavebeem
Copy link
Collaborator

Looks great now. Thanks for the contribution @ivan-kleshnin!

@ivan-kleshnin
Copy link
Contributor Author

No problem. Thank you for such a great library!

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.

None yet

3 participants