Skip to content
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

#6638 fixed table display type conversion #6650

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Conversation

lmitusinski
Copy link
Contributor

No description provided.

@scottdraves scottdraves merged commit 3423bad into master Jan 12, 2018
@@ -121,7 +121,9 @@ def convert_from_pandas(self, args, types_map):

@staticmethod
def convert_value(value, value_type):
if value_type == "time":
if value == "":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if a falsy test would be helpful here. The current code checks specifically for value being an empty string, but if the check was if value: we would catch None, an empty list, and other falsy values too.

Not sure if that's desirable, but certainly worth mentioning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty list should appear as [].
none sounds right though, what does it do now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of this function, it seems we return "string" for anything that doesn't match the specific checks. If we want to return "string" even when value is None, then the current code is all good. If we want to do something else when the value is None, we could do this at the at the end of the function.

Again, I'm not sure what the ideal behavior is, but I see a potentially unhandled case.

if value:
    return "string"
raise Exception("Unknown value type")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see also #6657 #6656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants