-
Notifications
You must be signed in to change notification settings - Fork 320
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
Add warnings that were suggested from metadata bughunt #2203
Conversation
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.
Looks good, I left two minor comments.
@@ -409,6 +409,8 @@ def update_columns(self, table_name, column_names, **kwargs): | |||
**kwargs: | |||
Any key word arguments that describe metadata for the columns. | |||
""" | |||
if not isinstance(column_names, list): |
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 wouldn't work for tuples and other list-like objects, something like pandas.api.types is_list_like
could handle those.
If that's expected then you can leave as is.
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 think that's expected. Don't imagine a user putting in something more complicated than a list of strings and it's easy enough to convert.
metadata_dict = {'tables': {table_name: table_metadata.to_dict()}} | ||
metadata = Metadata.load_from_dict(metadata_dict) |
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.
was this crashing?
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.
No it was causing the single table metadata name to be missing since it gave it a default table (it was in a SingleTableMetadata format)
Handle the remaining usability issues by adding warnings to let the user know that they will get a default table name.
Also add errors to prevent the user from adding invalid
SingleTableMetadata
arguments intoMetadata