search

pkgjs / parseargs

Polyfill of `util.parseArgs()`
Apache License 2.0
121 stars 9 forks source link

Refactor parsing for readability and future expansion, and tighten short option group parsing #68

Closed shadowspawn closed 2 years ago

shadowspawn commented 2 years ago

1) Refactor parsing:

Refactor heavily influenced by comments and code from @aaronccasanova in #64.

2) There is one functional change. A dubious option group with an option in the middle taking a value now returns the argument as a positional for BYO handling, and in strict mode this would throw. (The previous code was blindly expanding anything after a single -.) This behaviour influenced by comments from @ljharb, and narrower implementation of the Open Group Utility Conventions.

3) Separate out and extend tests for single - and for short option groups.

Apologies for the noise moving the tests, but something I badly wanted to do, and felt in scope for a big refactor!

First draft: no changes to unit tests. It works as before, as far as the tests went! Second draft: separate out and extend - unit tests Third draft: separate out and extend short option group tests

aaronccasanova commented 2 years ago

These updates are great @shadowspawn!! I'm honestly tempted to close out my PR in favor of this implementation. My goal was simply to swap out the conditionals with named utilities while introducing the smallest number of changes. That being said, I very much prefer the approach you took where each arg is assessed sequentially and the utility functions can stand on their own 👌

shadowspawn commented 2 years ago

Ready for review.

bcoe commented 2 years ago

@aaronccasanova, if you prefer this over #64, let's go with it?

Same comment stands as in #64, which is that I'd advocate that we keep these helpers private.

aaronccasanova commented 2 years ago

Yes, I prefer this implementation and agree the helpers should be private 👍 Wen't ahead and closed PR #64

shadowspawn commented 2 years ago

I am working on rewrite for after #63 lands, so changed this to draft.

shadowspawn commented 2 years ago

Working on redo of this with new configuration bag of options.

shadowspawn commented 2 years ago

Redid work in #75 after #63 landed.