-
Notifications
You must be signed in to change notification settings - Fork 93
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
chore(datasets): Remove deprecation warning from Table Dataset #956
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.
@deepyaman should filepath
and file_format
also be removed as arguments? I see in the history that you did remove them from the docstrings already.
Yes, please! |
Hi @deepyaman , I may not be fully aware of the context but what happens to the load function if we remove the parameters So to summarize, this will affect 2 functions Thank you |
All this logic can be removed; if self._filepath is not None:
if self._file_format is None:
raise NotImplementedError
reader = getattr(self.connection, f"read_{self._file_format}")
return reader(self._filepath, self._table_name, **self._load_args)
else: |
|
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.
Nevermind, I reviewed 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.
This largely looks good to me!
One more change that probably should be added is including the removal (and directions to use FileDataset
instead) in the release notes.
Since this is pretty much good, I wouldn't bother with making the changes (for the most part) until #909 gets merged and this is rebased.
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.
Approving, only minor suggestions
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!
Description
Remove deprecation warning as suggested here - Need to remove code that's marked as "to be removed": ibis dataset: https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/kedro_datasets/ibis/table_dataset.py#L123-L128
Development notes
Checklist
RELEASE.md
file