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

Able to delete in group for delete feature #103

Merged
merged 16 commits into from
Mar 27, 2024
Merged

Able to delete in group for delete feature #103

merged 16 commits into from
Mar 27, 2024

Conversation

cheahTJ
Copy link

@cheahTJ cheahTJ commented Mar 27, 2024

No description provided.

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 78.31633% with 85 lines in your changes are missing coverage. Please review.

Project coverage is 75.90%. Comparing base (8b62104) to head (0402c87).
Report is 120 commits behind head on master.

Files Patch % Lines
.../seedu/address/logic/commands/ScheduleCommand.java 43.75% 17 Missing and 1 partial ⚠️
...va/seedu/address/logic/commands/DeleteCommand.java 63.15% 10 Missing and 4 partials ⚠️
...ava/seedu/address/logic/commands/GroupCommand.java 79.31% 9 Missing and 3 partials ⚠️
...seedu/address/logic/parser/GroupCommandParser.java 69.23% 1 Missing and 7 partials ⚠️
...du/address/model/person/GroupMatchesPredicate.java 73.91% 3 Missing and 3 partials ⚠️
.../java/seedu/address/model/util/SampleDataUtil.java 0.00% 6 Missing ⚠️
...du/address/logic/parser/ScheduleCommandParser.java 64.28% 1 Missing and 4 partials ⚠️
...eedu/address/logic/parser/DeleteCommandParser.java 81.81% 1 Missing and 1 partial ⚠️
...in/java/seedu/address/logic/parser/ParserUtil.java 60.00% 1 Missing and 1 partial ⚠️
...du/address/model/person/EmailMatchesPredicate.java 86.66% 1 Missing and 1 partial ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #103      +/-   ##
============================================
+ Coverage     75.57%   75.90%   +0.33%     
- Complexity      439      529      +90     
============================================
  Files            72       82      +10     
  Lines          1392     1731     +339     
  Branches        132      183      +51     
============================================
+ Hits           1052     1314     +262     
- Misses          309      359      +50     
- Partials         31       58      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@hjuntan hjuntan 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 good to me. The stuff mentioned are things that can be improved on but it does not require immediate attention.

@@ -21,29 +25,69 @@ public class DeleteCommand extends Command {
public static final String MESSAGE_USAGE = COMMAND_WORD
+ ": Deletes the person identified by the NusId used in the displayed person list.\n"
+ "Parameters: NusId (8 digits long, starting with an 'E'). \n"
+ "Example: " + COMMAND_WORD + " E0123456";
+ "Example: " + COMMAND_WORD + " id/E0123456\n\n"
Copy link

Choose a reason for hiding this comment

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

Not sure if this is intentional but there are a double \n here

@@ -51,7 +51,7 @@ public void parseCommand_clear() throws Exception {
public void parseCommand_delete() throws Exception {
NusId testNusId = new NusId("E1234567");
DeleteCommand command = (DeleteCommand) parser.parseCommand(
DeleteCommand.COMMAND_WORD + " " + testNusId);
DeleteCommand.COMMAND_WORD + " id/" + testNusId);
Copy link

Choose a reason for hiding this comment

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

I think we can use the ParserUtil prefix instead of adding it ourselves

@hjuntan hjuntan added this to the v1.3 milestone Mar 27, 2024
@hjuntan hjuntan linked an issue Mar 27, 2024 that may be closed by this pull request
@hjuntan
Copy link

hjuntan commented Mar 27, 2024

Closes #51

@hjuntan hjuntan merged commit e09e858 into master Mar 27, 2024
8 checks passed
Comment on lines +41 to +43
if (arePrefixesPresent(argMultimap, PREFIX_NUSID)
&& !arePrefixesPresent(argMultimap, PREFIX_GROUP)
&& !arePrefixesPresent(argMultimap, PREFIX_TAG)) {

Choose a reason for hiding this comment

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

Maybe we really need to create a common method in ParserUtils that accepts an array of args

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.

Delete by group
4 participants