Update error message #216
Update error message #216
Conversation
…into update-errorMessage
There was a problem hiding this comment.
Pretty insane work you have done here, can really see the time and effort put into resolving the many possible cases. Good work even with the tests too, this isn't an extensive review but more of spotting random typos while trying to see how you handle other test cases which can be applied to the question you post in the description.
The solution I am proposing would be to check for valid prefixes by regex, which by default rejects anything else. Can discuss more about this during our next meeting.
| @@ -26,7 +26,8 @@ | |||
|
|
|||
| public static final String COMMAND_WORD = "find"; | |||
|
|
|||
| public static final String MESSAGE_USAGE = "Finds all supplier(s) or warehouse(s) whose name," | |||
| public static final String MESSAGE_USAGE = "Find Command Usage\n\nFinds all supplier(s) or warehouse(s)" | |||
| + " whose name," | |||
| + " remark and/or name of products matches any of the specified keywords (case-insensitive) and displays" | |||
| + " them as a list with index numbers. Prefixes provided can be in any order. At least one of the name," | |||
| + " remark or product prefixes along with its parameters must be provided.\n\n" | |||
There was a problem hiding this comment.
Small issue but extra space here, commenting in case I miss it later
| @@ -15,7 +15,8 @@ | |||
| public class RemoveMacroCommand extends Command { | |||
| public static final String COMMAND_WORD = "removemacro"; | |||
|
|
|||
| public static final String MESSAGE_USAGE = "Removes the macro identified by the alias used in the macro list.\n\n" | |||
| public static final String MESSAGE_USAGE = "Remove Macro Usage\n\nRemoves the macro identified by the" | |||
| + "alias used in the macro list.\n\n" | |||
| @@ -20,4 +20,42 @@ | |||
| public static final Prefix PREFIX_INDEX = new Prefix("i/"); | |||
| public static final Prefix PREFIX_ALIAS = new Prefix("a/"); | |||
| public static final Prefix PREFIX_COMMAND_STRING = new Prefix("cs/"); | |||
| public static final Prefix[] ALLOWED_PREFIXES = new Prefix[] { | |||
There was a problem hiding this comment.
Omg this is a lot of effort you put in. I'm thinking if we can use regex on this to pick out all the invalid prefixes
| } | ||
|
|
||
| if (!argMultimap.getValue(PREFIX_TYPE).isPresent()) { | ||
| throw new ParseException(String.format(MESSAGE_NO_PREFIX, EditCommand.MESSAGE_USAGE)); | ||
| // Check if arguments are valid |
There was a problem hiding this comment.
Good work including comments for better readability
| @@ -220,6 +252,88 @@ public static boolean arePrefixesPresent(ArgumentMultimap argumentMultimap, Pref | |||
| return Stream.of(prefixes).allMatch(prefix -> argumentMultimap.getValue(prefix).isPresent()); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Very good work abstracting all these out, it's still a headache that within each command there are so many different things to check for.
…into update-errorMessage
…into update-errorMessage
Finished standardizing all error messages for invalid arguments. Will need to do a minor change, after merging in Update Command from Qin Liang's PR due to the change from name to index prefix.
There is one bug, where if a user key in:
edit ct/s i/ z/testing => this will not capture z/testing as invalid prefix, since the input will be trimmed(?) and the space will be gone. Appreciate any advice on this :)