Open nus-pe-bot opened 1 year ago
Thanks for bringing this up.
TL;DR: Attempting to predict what a user intends by "1 2"
is a non-trivial task - does it mean "12"? "1" and "2"? The error message saying "Too many values" is pretty upfront with what is wrong with the input. The prediction of user intention to improve UX is beyond the scope of the feature. As required, the current behavior of the application fails gracefully with the reasonable error message and does not crash.
Since there are two parts to this, we'll address them separately.
For the first message, it already indicates that too many values have been provided, which should give enough information to the user to realize that they did provide too many indices, even before trying to add the quotes around it, which makes this a non-issue.
It is a general requirement in our commands to surround arguments with whitespace with quotes, and we decided to include this in our messages universally as this requirement is easily overlooked.
Now let's suppose the user chooses to ignore the first sentence, and we get the second error.
For the second error, the UG does specify that the index needs to be a positive integer. Because of the added quotes, "1 2"
becomes 1 value - the quotes tell us to treat whatever between them at face value and not try to interpret them unnecessarily (say what if the separator were a comma instead of a space? would "1,2"
be treated as 2 values? or maybe the space was unintentional and it's "12"? there are infinitely many possibilities for what the user intended here - too many to anticipate and validate).
Therefore, because "1 2"
is not an integer, the command rightfully flags the syntax error.
It is a non-trivial task to detect and predict user intention, and reasonable (from our point of view) to simply state that there are too many values right upfront, and then emit the syntax error message if the user proceeds further in the wrong direction.
Hence, the current behavior is is in fact the expected and correct behavior, and predicting user intent is beyond the scope of the feature. As required, the current behavior of the application fails gracefully with an already reasonable error message and does not crash.
also:
If you were to try remove task "1" "2"
then, now we get 2 values, the error message once again starts with "Too many values provided.", and we're back to square one, where it's pretty clear that there are too many values.
still not convinced? take a look at Unix commands:
sudo rm -rf /Library/Application Support
will delete everything in the /Library/Application
and Support
directories.
sudo rm -rf "/Library/Application Support"
(for obvious reasons don't run this command) will delete everything in just /Library/Application Support
.
Just because there's a space doesn't mean that the user intended the space as a separator, hence we must treat quoted arguments at face value and not try to interpret them to predict user intent.
--
The error message thrown for a particular case of
rm task
does not help user understand the mistake. In fact it might confuse them moreSteps to reproduce:
rm task 1 2
When quotation is used, new error message thrown does not help clarify mistake either.
Steps to reproduce:
rm task "1 2"
Usage: remove task(rm task) index."
[original: nus-cs2103-AY2223S1/pe-interim#5369] [original labels: type.FeatureFlaw severity.Low]