734 fix dataset schema #762
Conversation
simon-contreras-deel
requested review from
alrocar,
Jesus89 and
andy-esch
and removed request for
alrocar and
Jesus89
| if context: | ||
| self.cc = context | ||
| self.schema = context.get_default_schema() |
There was a problem hiding this comment.
If the user has already a schema and he calls the upload function with a different one, the original is modified. Is this the expected behavior?
There was a problem hiding this comment.
Yes, I have said in the description setting the dataset context implies having a new schema
There was a problem hiding this comment.
I mean:
new context == new api key and new username
So if we have a new username (or the same), we have to update the schema.
Using a different schema is needed to use data from users from your organization and I can not see a situation of conflict with this approach
There was a problem hiding this comment.
I have only changed it here because it is the only place updating the context. In develop we will need to do it in the context setter method
| else: | ||
| return 'public' | ||
|
|
||
| return None |
There was a problem hiding this comment.
This return None is not required
There was a problem hiding this comment.
Yep, but I want to be explicit
There was a problem hiding this comment.
I like redundant and explicit return Nones :)
There was a problem hiding this comment.
Curious about @Jesus89's comment https://github.com/CartoDB/cartoframes/pull/762/files#r292501862, otherwise I think it looks good
|
Green? |
Fix #734
public