nus-cs2103-AY2324S2 / forum

17 stars 0 forks source link

Questions regarding v1.4 constraints #719

Closed yashpola closed 2 months ago

yashpola commented 2 months ago
  1. If an illegal modification is made that flouts v1.4 and it's not possible to revert the PR or too tedious to rollback the commit, is it possible to make another PR removing the illegal mods without facing penalty?
  2. We had bugs that were reported regarding how interviews can be added where the end time precedes start time, and where the date precedes the current date. Is it okay to handle these erroneous user inputs because it may lead to incorrect/inconsistent data? And consequently add new error messages or expand current error messages for these cases.
damithc commented 2 months ago
  1. If an illegal modification is made that flouts v1.4 and it's not possible to revert the PR or too tedious to rollback the commit, is it possible to make another PR removing the illegal mods without facing penalty?

@yashpola Yes, that's fine.

2. We had bugs that were reported regarding how interviews can be added where the end time precedes start time, and where the date precedes the current date. Is it okay to handle these erroneous user inputs because it may lead to incorrect/inconsistent data? And consequently add new error messages or expand current error messages for these cases.

Tightening validity checks is an enhancement. Garbage-in garbage-out (i.e., if the app is given garbage values, it will use store/output the same garbage values) is not strictly an incorrect behavior. But if such values cause other thing to go haywire (e.g., corrupts the data file or crashes the application), then it is fine to modify the code to actively block them.

yashpola commented 2 months ago

@damithc Thanks prof, just 1 more question: Suppose I want to change which error message I want to throw upon an erroneous user input without adding or changing any error messages, would this be okay? I.e. if a user enters an invalid phone number, throw invalid phone exception instead of invalid command format. Since the latter may be misleading

damithc commented 2 months ago

Suppose I want to change which error message I want to throw upon an erroneous user input without adding or changing any error messages, would this be okay? I.e. if a user enters an invalid phone number, throw invalid phone exception instead of invalid command format. Since the latter may be misleading

@yashpola Making error messages more specific is not allowed. Switching of exception is still a code change, which is what we are trying to minimize during the feature freeze. But a change can be allowed if the current message is incorrect (i.e., gives incorrect information), not merely 'too general'.

If you think the current error message is incorrect , what's the current error message and what's the correct message?

yashpola commented 2 months ago

@damithc When a phone number is wrong, the current message is: "Invalid Command Format! Parameters: applicant_status [PHONE] s/[STATUS] Example usage: applicant_status 98362254 s/accepted"

A tester reported this as misleading since it doesn't inform that the phone number is wrong, but as if that the command itself was wrong ie missing some parameter etc

So we wished to switch to the existing "Phone numbers must contain 3 digits, and only numbers" error message

damithc commented 2 months ago

@yashpola We don't allow making error messages more specific. Given this error message can be misleading, we can allow you to add the missing parts e.g., Invalid Command Format OR Invalid Parameter Value In the UG, you can provide more information about which parameter validity.

yashpola commented 2 months ago

@damithc Understood thanks prof