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

[W5.11][T12-3] Davindran #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Davindran
Copy link

pls comment need class part thx

@nus-se-pr-bot
Copy link

Hi @Davindran, your pull request title is invalid.

For PR sent to satisfy a learning outcome, the PR name should be in the format of [Learning Outcome ID][Team ID] Your Name, where [Learning Outcome ID] has no dashes or spaces (e.g. [W3.1a]) and [Team ID] has one dash only and no spaces (e.g. [W14-2] means Wednesday 2pm (14 hrs), Team 2).
E.g. If you are in tutorial W09 (i.e. Wednesday 9am), team 1, and is submitting a PR for LO W2.2b, the PR title would be [W2.2b][W09-1] James Yong. Note that your tutorial IDs are different from those shown in CORS/IVLE.

For team PR, the PR name should be in the format of [Team ID] Product Name (e.g. [T09-2] Contact List Pro).

Please follow the above format strictly and edit your title for reprocessing.

Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR.

@Davindran Davindran changed the title Enhancement: 'find' function not case sensitive [W5.11][T12] Davindran Sep 12, 2018
@Davindran Davindran changed the title [W5.11][T12] Davindran [W5.11][T12-3] Davindran Sep 12, 2018
Copy link

@marlenekoh marlenekoh left a comment

Choose a reason for hiding this comment

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

@Davindran Good job at using branching and updating comments and documentation! Also nice that you gave your commit a meaningful name.

Please close this PR after reading review comments

@@ -79,17 +79,17 @@ Format: `find KEYWORD [MORE_KEYWORDS]`

[NOTE]
====
The search is case sensitive, the order of the keywords does not matter, only the name is searched,
The search is NOT case sensitive, the order of the keywords does not matter, only the name is searched,

Choose a reason for hiding this comment

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

good job updating docs! 👍

@@ -176,10 +176,20 @@ Example:

* `java seedu.addressbook.Main mydata.txt`

== ENHANCEMENT

Choose a reason for hiding this comment

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

Don't have to add this if you've mentioned it above in the find command description (:

@@ -11,13 +11,14 @@
/**
* Finds and lists all persons in address book whose name contains any of the argument keywords.
* Keyword matching is case sensitive.
* ENHANCEMENT: keyword matching is NOT case sensitive.

Choose a reason for hiding this comment

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

good job updating comments

@@ -14,7 +14,7 @@
|| Example: delete 1
|| Clears address book permanently.
|| Example: clear
|| find: Finds all persons whose names contain any of the specified keywords (case-sensitive) and displays them as a list with index numbers.
|| find: Finds all persons whose names contain any of the specified keywords (NON case-sensitive) and displays them as a list with index numbers.

Choose a reason for hiding this comment

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

good job updating tests (:

@@ -238,7 +238,7 @@ private int parseArgsAsDisplayedIndex(String args) throws ParseException, Number
* @return the prepared command
*/
private Command prepareFind(String args) {
final Matcher matcher = KEYWORDS_ARGS_FORMAT.matcher(args.trim());
final Matcher matcher = KEYWORDS_ARGS_FORMAT.matcher(args.toUpperCase().trim());

Choose a reason for hiding this comment

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

good (:

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.

3 participants