search

vimuthm / pe

0 stars 0 forks source link

Insufficient specificity of error message #3

Open vimuthm opened 2 years ago

vimuthm commented 2 years ago

When running update 1.2 e/3 (a scenario where user meant to type 12), a better error message would be Invalid anime index

image.png

nus-pe-bot commented 2 years ago

Team's Response

From AniList Devs:

As seen from the screenshot you provided, the error message states that the index "must be a positive integer". We believe that this is a specific enough error message to tell the user that the given index 1.2 is not valid as it is not a positive integer. We will be rejecting this as a bug.

image.png

However to implement this feature that was raised for us to detect 1.2 as a number would take a substantial amount of effort and possibly introduce more bugs into our application. As a result even if it were to be accepted it would have to be categorized as NotInScope. But that being said, we provided sufficient information for the error message and hence once again will not be accepting this bug.

image.png

Best Regards!

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: Thank you for your response! I disagree with this bug being rejected based on the following points:

  1. This error message is the generic message that's displayed when the command fails -> It is not user friendly to expect the user to scour the entire command format to realize which aspect was wrong (was it the index, the prefix or the episode number?)

  2. The application does give a specific error message for other indices such as 0 or a number greater than the existing number of anime as shown below. I believe that consistency in this case would be best. If some cases of an invalid index receive a well-phrased error message, all cases of invalid indices should.

image.png

  1. There was mention of substantial effort to fix this. I don't agree with this. With a quick glance at the codebase, I believe very few changes need to be made. The bug arises at this point:

image.png

The parseIndex method makes use of isValidNumber which uses an insufficient regex to be in tangent with it's name. Hence it's never caught in guard clause 40 where it should be (or even better a different exception to differentiate integers and non-integers).

image.png

All in all, I doubt this a "substantial effort" fix.

With these points in mind, I do not think this bug should be rejected.

Best Regards!