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.

Sign up for GitHub

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

Jump to bottom

fix(datasets): add metadata parameter to datasets #708

Merged
merged 2 commits into from
Jun 12, 2024
Merged

fix(datasets): add metadata parameter to datasets #708

merged 2 commits into from
Jun 12, 2024

Conversation

michal-mmm
Copy link
Contributor

Description

Fix issue: #699

Development notes

  • added metadata to __init__ of some datasets

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@michal-mmm
fix(datasets): add metadata parameter to datasets
8ffffd3 
Signed-off-by: michal-mmm <madej.michal@outlook.com>
@ankatiyar ankatiyar self-requested a review June 10, 2024 16:27
@ankatiyar ankatiyar added the Community Issue/PR opened by the open-source community label Jun 10, 2024
@ankatiyar ankatiyar requested a review from merelcht June 10, 2024 16:36
ankatiyar
ankatiyar approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

Thank you @michal-mmm! Good catch!

@ankatiyar ankatiyar requested a review from DimedS June 11, 2024 09:21
@noklam
Copy link
Contributor

noklam commented Jun 12, 2024

Is metadata a must now? Or does it do any harm if we force it to be part of AbstractDataset?

@datajoely
Copy link
Contributor

Is there any reason that the metadata constructor argument isn't part of the AbstractDataset definition?

@datajoely
Copy link
Contributor

lol same thought @noklam

@ankatiyar
Copy link
Contributor

@noklam @datajoely kedro-org/kedro#1076 (comment)
This is what I found 🤔

@ankatiyar ankatiyar merged commit 105fcf2 into kedro-org:main Jun 12, 2024
14 checks passed
@astrojuanlu astrojuanlu mentioned this pull request Jul 30, 2024
Closed
merelcht pushed a commit to galenseilis/kedro-plugins that referenced this pull request Aug 27, 2024
@michal-mmm @ankatiyar @merelcht
fix(datasets): add metadata parameter to datasets (kedro-org#708)
7fb97d2 
Signed-off-by: michal-mmm <madej.michal@outlook.com>
Co-authored-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankatiyar ankatiyar ankatiyar approved these changes

@noklam noklam noklam approved these changes

@merelcht merelcht Awaiting requested review from merelcht

@DimedS DimedS Awaiting requested review from DimedS

Assignees
No one assigned
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michal-mmm @noklam @datajoely @ankatiyar