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

feat: Add arg and option parsing utils #64

Closed
wants to merge 1 commit into from

Conversation

aaronccasanova
Copy link
Collaborator

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 bnb added this to the Merge into Node.js milestone Feb 18, 2022
@bnb
Copy link

bnb commented Feb 18, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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 🙅

index.js Show resolved Hide resolved
@aaronccasanova
Copy link
Collaborator Author

Closing in favor of #68

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.

4 participants