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

Add skeletal code for add and delete parser #118

Merged

Conversation

orzymandias
Copy link

@orzymandias orzymandias commented Oct 3, 2020

…into branch-common-parser

# Conflicts:
#	src/main/java/seedu/address/logic/commands/edit/EditCommand.java
#	src/main/java/seedu/address/logic/parser/AddressBookParser.java
#	src/test/java/seedu/address/logic/commands/EditCommandTest.java
@orzymandias orzymandias self-assigned this Oct 3, 2020
@orzymandias orzymandias added this to the v1.2b milestone Oct 3, 2020
@orzymandias orzymandias closed this Oct 3, 2020
@orzymandias orzymandias deleted the branch-common-parser branch October 3, 2020 18:01
@orzymandias orzymandias restored the branch-common-parser branch October 3, 2020 18:01
@orzymandias orzymandias reopened this Oct 3, 2020
Copy link

@seanjyjy seanjyjy left a comment

Choose a reason for hiding this comment

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

Overall looks great, just some nits to fix.

Just a side question as to would it be confusing as AddCommandParserWrapper is essentially Similiar to DeleteCommandParser, however, they have different naming syntax.
Is this implementation temporary in order to allow ab3 to function properly?

public class DeleteCommandParser implements Parser<DeleteCommand> {
public class DeleteCommandParser implements Parser<DeleteCommandAbstract> {

private static final int ITEM_TYPE_POS = 0;
Copy link

Choose a reason for hiding this comment

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

If most to all parser requires these static final int variables, we could shift it to avoid duplicates. However, I am not sure if it is apt to shift it to an interface. Can consider having a Util class for it.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good! I was thinking of doing so after the other parsers are done since for example the EditParser for the InternshipCommand may want to abstract 3 items (comindex, index ,prefixes)? So it depends on implementation of the parsers first then after which we deal with abstraction for common vars.

@orzymandias
Copy link
Author

Overall looks great, just some nits to fix.

Just a side question as to would it be confusing as AddCommandParserWrapper is essentially Similiar to DeleteCommandParser, however, they have different naming syntax.
Is this implementation temporary in order to allow ab3 to function properly?

For AddCommandParserWrapper each item has its own set of parsing rules which may require their own individual parsers so the AddCommandParserWrapper is only responsible for funneling the appropriate item prefix to its add parser. The thing about delete command parser is that there is lesser parsing involved except for extracting out the index to be deleted. With the exception of internship which has an additional index required, all the other commands just need the index extracted to be passed to their delete commands. So I am not sure there is a need a wrapper in that case.

As for consistency I think once AddCommandParser can be safely removed, we can rename AddCommandParserWrapper to AddCommandParser.

Copy link

@seanjyjy seanjyjy left a comment

Choose a reason for hiding this comment

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

LGTM good job.

@seanjyjy seanjyjy merged commit 78032a2 into AY2021S1-CS2103T-T15-4:master Oct 4, 2020
@seanjyjy seanjyjy modified the milestones: v1.2b, v1.2 Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skeletal code for Add and Delete Parsers
2 participants