-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[FEATURE] Add DataContext.update_datasource
CRUD method
#5417
[FEATURE] Add DataContext.update_datasource
CRUD method
#5417
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
👇 Click on the image for a new way to code review
Legend |
@@ -1913,6 +1913,22 @@ def _instantiate_datasource_from_config( | |||
) | |||
return datasource | |||
|
|||
def update_datasource( |
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 feels like there is more to do here, and that maybe we don't want to pass a config object here but a datasource one. If someone grabs a context with a datasource, makes changes, how do they save it? Other methods work to maintain the cached datasource collection, is that same effort needed here?
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 agree! I've modified the signature to take in an actual datasource.
Additionally, I've made it such that BaseDataContext
doesn't persist by default and DataContext
does. The cache has also been updated.
Let me know if this is closer to what you're envisioning.
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!
Args: | ||
datasource_name: The name of the Datasource to update. | ||
datasource_config: The config object to persist using the DatasourceStore. | ||
""" |
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.
Do you want to add a Raises
section here or do you not want to bake that into the API?
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.
That's a good question! I'm not sure what is a best practice here. I've leaned towards only adding Raises
if an exception is raised within the top level scope of the function body.
Any preference?
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.
There is a small comment about this made in the Google python style guide:
https://google.github.io/styleguide/pyguide.html#doc-function-raises
My reading of it is If you want to codify that raising this exception is part of this functions API and callers should be handling it then document it, otherwise don't. I'm not sure where/how this is planning on being used.
datasource_name=datasource_name, datasource_config=updated_datasource_config | ||
) | ||
|
||
key: DataContextVariableKey = DataContextVariableKey( |
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.
My preference would be to remove the annotations on key
and actual_config
since you instantiate/cast it in the same line and mypy
will infer this.
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 agree! We've been leaning towards extra verbosity but this is quite obvious when we see the constructor call in the same line. Updating now.
resource_name=datasource_name, | ||
) | ||
actual_config: DatasourceConfig = cast( | ||
DatasourceConfig, datasource_store_with_single_datasource.get(key=key) |
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.
Just looked and the backing store for this config is a dict and not typed 😢
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.
Ugh yeah our typing needs some work - lots of bags of data that could use structure. We've been incrementally adding dataclasses to provide some of that type safety but it's a work in progress.
…_expectations into feature/great-888/great-1037/DataContext.update_datasource
…_expectations into feature/great-888/great-1037/DataContext.update_datasource
Args: | ||
datasource_name: The name of the Datasource to update. | ||
datasource_config: The config object to persist using the DatasourceStore. | ||
""" |
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.
There is a small comment about this made in the Google python style guide:
https://google.github.io/styleguide/pyguide.html#doc-function-raises
My reading of it is If you want to codify that raising this exception is part of this functions API and callers should be handling it then document it, otherwise don't. I'm not sure where/how this is planning on being used.
def update_datasource( | ||
self, | ||
datasource_name: str, | ||
datasource: Union[LegacyDatasource, BaseDatasource], |
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.
datasource
has a name
property so passing datasource_name
now seems redundant.
def update_datasource( | ||
self, | ||
datasource_name: str, | ||
datasource: Union[LegacyDatasource, BaseDatasource], |
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.
Similar comment here about datasource_name
.
Changes proposed in this pull request:
update_datasource
method to enable updating of existing datasources from notebooksDatasourceStore
Definition of Done
Please delete options that are not relevant.
Thank you for submitting!