Add group command #100
Add group command #100
Conversation
Group multiple users in single command
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #100 +/- ##
============================================
+ Coverage 76.29% 76.86% +0.56%
+ Complexity 545 535 -10
============================================
Files 84 82 -2
Lines 1772 1716 -56
Branches 187 176 -11
============================================
- Hits 1352 1319 -33
+ Misses 360 340 -20
+ Partials 60 57 -3 ☔ View full report in Codecov by Sentry. |
| @@ -48,101 +47,116 @@ public class GroupCommand extends Command { | |||
| + PREFIX_GROUP + "Class T15 " | |||
| + PREFIX_TAG + "TA"; | |||
|
|
|||
| private final NusId toGroup; | |||
| private final Set<NusId> toGroup; | |||
| * If {@code nusIds} contain only one element which is an empty string, it will be parsed into a | ||
| * {@code Set<NusId>} containing zero nusId. | ||
| */ | ||
| private Optional<Set<NusId>> parseNusIdsForGroup(Collection<String> nusIds) throws ParseException { |
There was a problem hiding this comment.
I don't think there is a need to wrap the set of NUSID in an optional, when you are going to get the set later on and using it to filter the person list.
| nusIds.add(person.getNusId()); | ||
| } | ||
|
|
||
| GroupCommand.GroupPersonDescriptor descriptor = new GroupPersonDescriptorBuilder(groupedPersons).build(); |
There was a problem hiding this comment.
I think you can import the nested class into the test class instead of using the dot operator
|
|
||
| String userInput = " id/" + nusId + GROUP_DESC_HUSBAND + TAG_DESC_AMY; |
There was a problem hiding this comment.
We didn't catch this earlier; but we should use the prefixes made in logic.parser.CliSyntax instead of adding it as a String itself
| String defaultNusId = "E1234567"; | ||
| NusId nusid = new NusId((defaultNusId)); | ||
| NusId nusId = new NusId((defaultNusId)); |
| } | ||
|
|
||
| @Test | ||
| public void execute_invalidPersonIndexFilteredList_failure() { | ||
| public void execute_partialInvalidPersonsNusIdFilteredList_failure() { |
There was a problem hiding this comment.
nit: might be nice to explain what this testcase does since "partial invalid" is not immediately clear what the test does
Fix missing messages after merge conflict
|
Closes #92 |
No description provided.