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

Update user guide to clarify confusing behaviour raised during PE #214

Merged
merged 10 commits into from
Nov 7, 2020

Conversation

csiongn
Copy link

@csiongn csiongn commented Nov 3, 2020

Fixes #153
Fixes #154

@csiongn csiongn added this to the v1.4 milestone Nov 3, 2020
Copy link

@ong-yinggao98 ong-yinggao98 left a comment

Choose a reason for hiding this comment

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

Do check out my comments!

@@ -215,7 +215,7 @@ Examples:

#### 3.2.4 Listing all students: `list`

You can view the list of all students in **Reeve**.
You can view the list of all students in **Reeve** in order sorted by the name of the student..

Choose a reason for hiding this comment

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

A better way to phrase this would be "list of all students [...] sorted by name".

@@ -559,7 +559,7 @@ Action | Format, Examples
--------|------------------
**Add Student** | `add n/NAME p/PHONE s/SCHOOL y/YEAR v/CLASS_VENUE t/CLASS_TIME f/FEE d/LAST_PAYMENT_DATE [a/ADDITIONAL_DETAILS]...​` <br> e.g., `add n/John Doe p/98765432 s/Woodlands Secondary School y/Secondary 2 v/347 Woodlands Ave 3, Singapore 730347 t/1 1200-1400 f/30 d/24/09/2020 a/Likes chocolates a/Needs help with Algebra`
**Edit Student** | `edit STUDENT_INDEX [n/NAME] [p/PHONE] [n/NAME] [p/PHONE] [v/CLASS_VENUE] [s/SCHOOL] [sb/SUBJECT] [y/YEAR] [t/CLASS_TIME]`<br> e.g.,`edit 1 n/Alex p/99999999 s/Meridian Junior College`
**Find Student** | `find [n/NAME] [s/SCHOOL] [y/YEAR] [sb/SUBJECT]`<br> e.g., `find n/alex s/yishun`
**Find Student** | `find [n/NAME] [s/SCHOOL] [y/YEAR]`<br> e.g., `find n/alex s/yishun`

Choose a reason for hiding this comment

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

Could you just remove the whitespace at the end of this line?

Comment on lines 203 to 207
* The order of the optional fields do not matter. e.g `n/Hans s/River Valley` will match `s/River Valley n/Hans`
* Only full words will be matched. e.g `han` will not match `hans`.
* For the name, students with a name that matches any whole keyword specified for the name will be considered to match for the name.
* For the school, students with a school that contains any keyword specified for the school will be considered to match for the school.
* For the school, students with a school that contains all keyword specified for the school will be considered to match for the school.
* Only students matching all criteria specified will be returned (i.e `AND` search).

Choose a reason for hiding this comment

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

You might have to rewrite this whole segment, considering that this is no longer that accurate:

  1. full word match only applies to name search
  2. year search has not been explained

Perhaps for reference, 204-206 could be:
"For name search, only students whose names contain at least one of the given keywords will be matched. Only full words will be matched, e.g han will not match hans."
"For school search, only students whose school contains the entire given keyword will be matched, e.g Yishun will match Yishun Sec but not Yishun Secondary."

Copy link
Author

Choose a reason for hiding this comment

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

Okay thanks for pointing the potential misreading in line 204! For line 206 I don't get what you mean by "e.g Yishun will match Yishun Sec but not Yishun Secondary." as Yishun currently will match Yishun Secondary.

Copy link
Author

@csiongn csiongn Nov 3, 2020

Choose a reason for hiding this comment

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

Also, would "the entire given keyword" brings up the possibility of confusion as this can be interpreted to mean that the entire keyword string must be found in one chunk.

Choose a reason for hiding this comment

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

Hmmm... perhaps it could be
"e.g Yishun will find all instances of Yishun Sec but Yishun Secondary will not."
and "whose school contain the entire set of given school keywords"

Comment on lines 271 to 273
* For the name, students with a name that matches any whole keyword specified will be matched.
* For the school, students with a school that contains all keywords specified will be matched.
* For the year, only students with the same year will be matched. (See below for more notes)
Copy link

Choose a reason for hiding this comment

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

Perhaps it'd be clearer if you phrased it as "For xxx search"
e.g "For name search", "For school search" and "For year search"
"with a name that" and "with a school that" could be further shortened to "whose name/school"

I think by "notes" you mean "examples"?

Copy link
Author

@csiongn csiongn Nov 7, 2020

Choose a reason for hiding this comment

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

Oops I left it out by accident. I meant more elaboration for how the year is parsed and interpreted by Reeve. Hmm I feel saying "for name search", "for school search" etc feels like there is only one criteria they can use instead of multiple as in our app. If it's better, I've just changed it to "for the name criteria" etc!

@@ -268,28 +268,19 @@ Format: `find [n/NAME] [s/SCHOOL] [y/YEAR]`
* At least one of the optional fields must be provided.
* The order of the optional fields do not matter. e.g `n/Hans s/River Valley` will match `s/River Valley n/Hans`
* Only full words will be matched. e.g `han` will not match `hans`.

Choose a reason for hiding this comment

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

You will still have to update this line because it is specific to name-search

Copy link
Author

Choose a reason for hiding this comment

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

Right! Nice catch!

@codecov-io
Copy link

codecov-io commented Nov 7, 2020

Codecov Report

Merging #214 (eb47e33) into master (429f7ec) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #214   +/-   ##
=========================================
  Coverage     78.53%   78.53%           
  Complexity     1155     1155           
=========================================
  Files           157      157           
  Lines          3503     3503           
  Branches        407      407           
=========================================
  Hits           2751     2751           
  Misses          611      611           
  Partials        141      141           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 429f7ec...eb47e33. Read the comment docs.

docs/UserGuide.md Outdated Show resolved Hide resolved
@ong-yinggao98 ong-yinggao98 merged commit aa5eba3 into AY2021S1-CS2103T-W15-2:master Nov 7, 2020
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.

[PE-D] Sorting and listing [PE-D] Find feature keyword restrictions
3 participants