-
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
Fix cattag duplicate tag within command #257
Fix cattag duplicate tag within command #257
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
**[Add](#adding-a-person-add)** | `add n/NAME p/PHONE_NUMBER e/EMAIL [t/TAG]…` <br> e.g., `add n/James Ho p/91231234 e/[email protected] t/friend t/classmate` | ||
**[Clear](#clearing-all-entries--clear)** | `clear` | ||
**[Delete](#deleting-a-person--delete)** | `delete INDEX`<br> e.g., `delete 3` | ||
**[Edit](#editing-a-person--edit)** | `edit INDEX [n/NAME] [p/PHONE_NUMBER] [e/EMAIL] [t/TAG]…`<br> e.g.,`edit 2 n/James Lee e/[email protected]` | ||
**[Find by contact information](#locating-persons-by-contact-information-find)** | `find PREFIX/KEYWORD [PREFIX/MORE_KEYWORDS]…`<br> e.g., `find n/James t/floorball` | ||
**[Delete tag](#deleting-a-persons-tag--deltag)** | `deltag INDEX t/KEYWORD` <br> e.g. `deltag 1 t/friend` | ||
**[Add tag](#adds-tags-to-a-specific-person--addtag)** | `addtag INDEX t/KEYWORD [t/MORE_TAGS]…` <br> e.g. `addtag 1 t/friend t/classmate` | ||
**[Categorize tag](#categorizing-a-tag--cattag)** | `cattag t/TAG [t/MORE_TAGS…] CATEGORY` <br> e.g. `cattag t/floorball t/mahjong activity` | ||
**[Undo action](#undo-a-command--undo)** | `undo` | ||
**[Redo action](#redo-a-command-redo)** | `redo` | ||
**[List](#listing-all-persons--list)** | `list` | ||
**[Help](#viewing-help--help)** | `help` |
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.
Look better
* `cattag t/CS2100 acads` categorizes the tag `CS2100` under `Academics` and display colour of the tag`CS2100` becomes `Gold`. | ||
* `cattag t/floorball t/mahjong activity` categorizes both tags `floorball` and `mahjong` under `Activities` with colour `Blue`. | ||
* Newly created tags (by [`add`](#adding-a-person-add) or [`addtag`](#adds-tags-to-a-specific-person--addtag)) will have category `General` and colour `Grey` by default. |
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.
Better details
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
@@ -66,7 +68,7 @@ private boolean isDuplicateCategory(TagCategory cat) { | |||
return updatedCategory.equals(cat); | |||
} | |||
|
|||
private String formatTags(List<Tag> tags) { |
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.
the use of set will solve duplicate tag expectedly.
This should:
cattag t/tag1 t/tag1 catA
as a valid command, and settag1
tocatA
successfully (if all other conditions are satisfied). Fixes Cattag bug when calling the same tag multiples time in one command #254