-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Raise error for enum creation from invalid string values #1300
Conversation
Signed-off-by: Partho Sarthi <[email protected]>
Signed-off-by: Partho Sarthi <[email protected]>
Signed-off-by: Partho Sarthi <[email protected]>
Signed-off-by: Partho Sarthi <[email protected]>
'Accepted options are: [%s].\n\t' | ||
'Falling-back to default filter: %s', | ||
arg_val, Utils.gen_joined_str(' | ', available_filters), | ||
QualFilterApp.tostring(selected_filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this behavior the same? Previously, this block does not raise exception even if arg_val
is not QualFilterApp
, but after this changes, it could.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check seemed redundant to me. QualFilterApp
argument is provided by the user, and our argument validator (argprocessor) would validates these inputs as first step. If there are invalid values, we will terminate early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was initially there because we had the legacy CLI that was not doing the same initialization as spark_rapids
.
There pros-cons removing it:
- pros: it is redundant
- cons: Starting the qualification tool without going through the argProcessor will bypass all those checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in that case, it will bypass those checks. If caller provides invalid value of QualFilterApp
, the tool would crash instead of silently continuing with the default value. I think this should be the right behavior. I agree this differs from the original behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @parthosa!
Fixes #887: This PR updates the
fromstring()
method for Enum creation to raise aValueError
for invalid input strings. Previously, it would silently returned None, which could lead to unexpected behaviour if not handled by the caller.Changes
class
EnumeratedType
:fromstring()
now raises a ValueError for invalid strings.fromstring()
return type to EnumeratedType.get_default()
to return a default value. This is implemented by the subclasses.