-
Notifications
You must be signed in to change notification settings - Fork 1
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
Tried to add multiple arguments to options #33
base: main
Are you sure you want to change the base?
Conversation
Sorry @dXu23, didn't see that you made this ready for review. however im kind of confused because the code has commented out console log statements. still working on this? |
Sorry about that. I kind of forgot to remove the console.log statements. |
let lastOptionObject: IOption | null = null; | ||
const optionsProcessed: string[] = []; | ||
|
||
passedInOptions.forEach((option: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dXu23 , again sorry for being late. don't get much time to look at the lower priority modules. anyways, any reason you went with different loops as opposed to keeping the .forEeach here? i ask because from a readability point of view, it's difficult for me to keep track of what's being iterated. im all for different loops, but reason i chose for each was so that i didn't have to add [i] to retrieve values. wondering if you found this approach to be better
optionObject.takes_value = true; | ||
const sig = signature.substring(0, openBracketNdx).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question here. what's the reason behind tokenizing the string in this else block? is it mainly for validating that the option is written properly so it can be parsed? simply trying to get context here so i can better understand the goal and if there may be another way than tokenizing
Closes #15
So far the following greet.ts appears to work
Changes not final and tests need to be written. It might be better to use regexes here.