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

Refactor the "Prefix" class to "Flag", and update the defaults to match the Unix-like syntax "--flag" #28

Merged
merged 10 commits into from
Oct 10, 2023

Conversation

wxwern
Copy link

@wxwern wxwern commented Oct 6, 2023

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.

wxwern added 3 commits October 7, 2023 00:38
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.
@wxwern wxwern added this to the v1.2 milestone Oct 6, 2023
@wxwern wxwern added the type.Chore Something to do but not a story, feature, bug or documentation (CS2103T label) label Oct 6, 2023
@wxwern wxwern marked this pull request as ready for review October 6, 2023 17:38
@wxwern wxwern requested review from a team, tanshiyu1999 and wamps-jp and removed request for a team October 6, 2023 17:38
@wxwern wxwern enabled auto-merge October 6, 2023 17:38
wxwern added 2 commits October 7, 2023 14:07
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.
@wxwern wxwern changed the title Refactor the "Prefix" class to "Flag", and update the defaults to match the Unix-like syntax "--flag". Refactor the "Prefix" class to "Flag", and update the defaults to match the Unix-like syntax "--flag" Oct 7, 2023
@wxwern wxwern disabled auto-merge October 7, 2023 06:27
Copy link

@tanshiyu1999 tanshiyu1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@wamps-jp wamps-jp left a 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!

@wxwern wxwern requested a review from McNaBry October 9, 2023 14:45
Copy link

@McNaBry McNaBry left a 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?

@wxwern
Copy link
Author

wxwern commented Oct 10, 2023

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 Flag now itself has a prefix and postfix field separate from the name - that is intentional to split command syntax from the actual name of a flag.

@wxwern wxwern requested a review from McNaBry October 10, 2023 09:45
Copy link

@McNaBry McNaBry left a 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.

@McNaBry McNaBry merged commit 2e5b56d into AY2324S1-CS2103T-W08-3:master Oct 10, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Chore Something to do but not a story, feature, bug or documentation (CS2103T label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update command argument syntax to use -- prefix instead of / postfix
4 participants