Adjust add-on input validation

[AhdraMeraliQB]

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Following #3185, this PR ensures that invalid inputs to the add-on prompt fail immediately. Additionally, case sensitivity has been removed, whitespaces are now ignored, and selections larger than 9 are processed as invalid selections instead of improper formatting.

The case sensitivity changes have also been made to the add-on flag inputs (they were already whitespace robust prior to this PR).

Development notes

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[merelcht]

Note: Values provided to the add-on flag remain case sensitive after these changes.

I would expect this to also have some processing to handle cases, the flows aren't entirely consistent, but I think this behaviour should be the same.

[merelcht]

I also noticed that if I put in a negative number I still get the unhelpful message: ValueError: invalid literal for int() with base 10: ''

[AhdraMeraliQB]

@merelcht

I would expect this to also have some processing to handle cases, the flows aren't entirely consistent, but I think this behaviour should be the same.

Done! Addressed in a1fb0c6

I also noticed that if I put in a negative number I still get the unhelpful message: ValueError: invalid literal for int() with base 10: ''

Fixed in 906f63a

[noklam]

Some of these tests are repetitive, can it be reduce some of these to one example. For example

            "ALL",
            "All",
            "AlL" 

can be reduced to one since AlL is probably the most weird case?

[AhdraMeraliQB]

I do agree AlL is probably the strangest case, though I'd avoid simplifying to just either ALL or All as I'd want to ensure all small letters, all capitals, and a mix of the two are all processed correctly. AlL and nonE have both been dropped in 5ad8bab

[noklam]

Looks good in general, leave a minor comment to reduce the test cases, each test case should be testing one specific thing.

[merelcht]

There's still a difference in user experience when I enter a negative number vs a positive invalid number:

This might be fine, because - is also used for range here. @amandakys do you have any thoughts on this?

[amandakys]

@merelcht Do you mean to say that a negative value should be accepted and auto corrected to be a positive value? Only when it falls within correct range, i.e we should validate the absolute value?

[AhdraMeraliQB]

@amandakys I think @merelcht is referring to the fact that they both invalid options but inconsistent errors are returned depending on what the input is, rather than processing any negative numbers as positive.

Currently the input is received as a string, negative numbers are rejected as they are read as an incompletely defined range, which doesn't fit any of the expected input patterns. If the input string does match one such pattern and is processed into individual selections, these selections are evaluated for whether they exist, resulting in the second type of error message.

I personally don't see any issues with the differing error messages as the errors are indeed thrown for different reasons (no negative number will ever be acceptable vs a limited but growing list of selections). Additionally if we were to change the response to -x would the response to x- change as well? (both are currently disallowed by the regex and result in the first error message)

[noklam]

I suggest test it after solving the conflicts. I suspect this maybe solved already as a side-effect of #3265. I introduced a new dictionary and use it everywhere, this avoids an addition rules because the condition it checking if value in the dictionary thus handle edge cases like negative value.

[merelcht]

I found one more interesting edge case, what if you put a range that's way beyond the acceptable number, e.g. "5-12", now you'll get:

Maybe I'm going too far into what users could reasonably enter, but I don't see why this shouldn't just throw: "'12' is not a valid selection.
Please select from the available add-ons: 1, 2, 3, 4, 5, 6, 7."

[noklam]

@merelcht This is a reasonable expectation, I have made the changes and updated the tests.

[SajidAlamQB]

Shouldn't we ignore white spaces? :

[noklam]

@SajidAlamQB Did you test with latest branch? There are test cases covering that already and it seems working fine on my end.

[SajidAlamQB]

@SajidAlamQB Did you test with latest branch? There are test cases covering that already and it seems working fine on my end.

I created a new environment and it seemed to work there. Perhaps my old environment had some issues... 😅

[merelcht]

LGTM! 👍

[SajidAlamQB]

Thank you, @noklam @AhdraMeraliQB!

[noklam]

Thank you @AhdraMeraliQB for implementing the solution 😸 I just help to fix a test.