-
Notifications
You must be signed in to change notification settings - Fork 4
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
Modify validation of names and related tests #37
Modify validation of names and related tests #37
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #37 +/- ##
============================================
- Coverage 56.42% 56.37% -0.05%
+ Complexity 478 477 -1
============================================
Files 97 97
Lines 2047 2047
Branches 236 236
============================================
- Hits 1155 1154 -1
Misses 841 841
- Partials 51 52 +1 ☔ View full report in Codecov by Sentry. |
// name has trailing spaces, all other attributes same -> returns false | ||
String nameWithTrailingSpaces = VALID_NAME_BOB + " "; | ||
editedBob = new PatientBuilder(BOB).withName(nameWithTrailingSpaces).build(); | ||
assertFalse(BOB.isSamePatient(editedBob)); |
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.
why is this testcase being removed?
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 new regex match string does not allow for trailing spaces. This is beacuse there is no String#trim() being called in the constructor of the Name object, and it is the responsibility of the parser to remove trailing spaces. For example if some route takes in user input " NAME " as the string and does not call String#trim(), that should not count as a valid name.
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 except some minor details
|
||
// valid name | ||
assertTrue(Name.isValidName("peter jack")); // alphabets only | ||
assertTrue(Name.isValidName("12345")); // numbers only |
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.
Why is this case being removed?
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.
Because we're disallowing numbers. If you mean that it should be moved to a negative test, I could do that
…into branch-bugfix-22
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.
84ac2a0
into
AY2324S2-CS2103T-T14-2:master
close #22
close #36
Name format is now as follows:
Alphabet, repeat :special character followed immediately by alphabet OR just alphabet
Special characters allowed:
\
/
-