pkgjs / parseargs

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

feat: Add arg and option parsing utils #64

Closed aaronccasanova closed 2 years ago

aaronccasanova commented 2 years ago

This PR introduces a number of argument and option parsing utilities. I thought this might help the understanding of the control flow/branching logic used to determine if an argument is a positional, long option, short option, etc. (This came out of a bit of frustration reading through the implementation and having to retain context of prior conditionals to understand the state of the current arg)

I used terminology found in the Utility Syntax Guidlines and include ESDoc style comments to each utility (with links) to enhance the editor/developer experience: arg-utils

bnb commented 2 years ago

Added the Merge into Node.js milestone, not because this needs to be merged by then but because IMO we should definitely figure this out - merge or close - before that IMO.

shadowspawn commented 2 years ago

An assortment of comments. I'm not expecting you make changes, this is more feedback on the concept. 😄

References! ESDoc! Raising the bar! 🎉

isStdInOutPositional: (in my opinion) the high level reason we treat a single dash as a positional is because it can be used as such and throwing an error would be overkill and not particularly useful. A specific documented use of a single dash is to represent stdin/stdout when a positional file name is expected. So the isStdInOutPositional routine and description are self-consistent, but it isn't the only reason we allow a single dash in the parsing so the usage in the code feels slightly misleading.

You commented that you were having to keep context as you read the code which is a fair complaint, and it could be clearer. However, you still have if-then-else-if code which assumes context, and the routines do not stand alone. For example isOption('-') and isOption('--') will both return true.

Exporting the routines increases the public API a lot, and would require tests. Using them internally for clarity may be useful on its own in some cases.

(I am thinking about some PR around non-trivial use of short options, and your comments and code are influencing how I am likely to approach them.)

aaronccasanova commented 2 years ago

Thanks for the feedback @shadowspawn

So the isStdInOutPositional routine and description are self-consistent, but it isn't the only reason we allow a single dash in the parsing so the usage in the code feels slightly misleading.

Agreed, I was forcefully trying to give a name to every arg. This could easily remain arg === '-' without adding to the contextual overhead of the branching logic.

However, you still have if-then-else-if code which assumes context, and the routines do not stand alone.

Absolutely, good point 👍 I more or less left everything where it was originally and created utility functions to improve readability.

For example isOption('-') and isOption('--') will both return true.

Oh wow, this is a total oversight 😄 I won't do this now (until we get more feedback) but we could fix this issue by updating the implementation to: const isOption = arg => /^--?\w+/.test(arg)

Exporting the routines increases the public API a lot, and would require tests.

Yeah, let's not do this now 🙅

aaronccasanova commented 2 years ago

Closing in favor of #68