-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor the "Prefix" class to "Flag", and update the defaults to match the Unix-like syntax "--flag" #28
Refactor the "Prefix" class to "Flag", and update the defaults to match the Unix-like syntax "--flag" #28
Conversation
This is done in preparation for the syntax change for a flag, formerly prefix, from f/ to --f, to make it in line with common CLI naming. Additionally, future functionality includes prefix-matching of a flag, which will have naming conflicts and confusions if this is not renamed.
A future update will allow for prefix-only matching separately. For now, this is done to reduce ambiguity and allow for more flags to be defined with no conflicts.
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.
LGTM
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.
Should be fine for now!
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.
Verified changes to parser works for add and edit. Since Prefix is being refactored to flags, do we want to change all mentions of prefixes to flags similar to the person -> contact refactor?
Yeah, I missed a few and have fixed them in the latest commit. Note that |
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.
Ok the refactor seems to have changed any mentions of the old Prefix class in the source code. Overall, looks good to me.
The renaming is primarily done because it may conflict with future prefix-matching functionality planned. This also makes the class use a common name for easier understanding (it is commonly called a command-line option, flag, or switch).
The refactor also includes some improvements in internal structure to allow for flexible syntax changes.
The update of the syntax (e.g., from
n/
) to the Unix-like variant (e.g., to--name
) closes #16.Note: The existing code does not require a whitespace between flags though, which may be confusing since something like
--nameAlice
could be accepted. This will be addressed in a separate PR to revamp the argument parsing.