-
Notifications
You must be signed in to change notification settings - Fork 135
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
Handle empty input parameter. #158
Conversation
Signed-off-by: Mohammad Nassar <[email protected]>
I am not sure what exactly this fixes. Empty params are processed correctly through the set of default vaues |
res += f"--{key} " | ||
elif isinstance(value, str): | ||
if value != "": | ||
res += f'--{key}="{value}" ' |
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.
what if the value has double-quotes in it?
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.
It shouldn't have double-quotes. For example, the dictionary keys and values should be quoted with single-quote.
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.
What will be if the value is None?
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.
I suggest to use
if len(value) != 0:
res += f'--{key}="{value}" '
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.
It shouldn't be None
value here, because if the user keep the input as blank field it will be an empty string. Also len(None)
will throw an exception.
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.
+1 Mohhammad
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.
After thinking about it some more I think it is even simpler. Considering that the code has proper defaults for everything, I thing we can do this:
for key, value in d.items():
if str(value) != "":
if isinstance(value, str):
res += f'--{key}="{value}" '
else:
res += f"--{key}={value} "
return res
So we are just removing parameters with empty value
The purpose of this is to "ignore" the empty input parameters and don't include them in the command line invocation of the transform so it will use the default values as defined by the library. |
res += f"--{key} " | ||
elif isinstance(value, str): | ||
if value != "": | ||
res += f'--{key}="{value}" ' |
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.
What will be if the value is None?
res += f"--{key} " | ||
elif isinstance(value, str): | ||
if value != "": | ||
res += f'--{key}="{value}" ' |
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.
I suggest to use
if len(value) != 0:
res += f'--{key}="{value}" '
res += f"--{key} " | ||
elif isinstance(value, str): | ||
if value != "": | ||
res += f'--{key}="{value}" ' |
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.
+1 Mohhammad
Signed-off-by: Mohammad Nassar <[email protected]>
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.
LGTM
Why are these changes needed?
Related issue number (if any).