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

feat: Implement plotly.HTMLDataset #788

Merged
merged 8 commits into from
Aug 22, 2024
Merged

feat: Implement plotly.HTMLDataset #788

merged 8 commits into from
Aug 22, 2024

Conversation

yury-fedotov
Copy link
Contributor

@yury-fedotov yury-fedotov commented Jul 27, 2024 •
edited
Loading

Description

Closes #787

Development notes

  • Given that this dataset is very similar to existing plotly.JSONDataset, I've added it as core. Though let me know if you prefer it to be experimental.
  • I tried to keep the test file as similar to plotly.JSONDataset as possible.

There are just two differences with plotly.JSONDataset:

  • This one cannot load at all because plotly doesn't support anything like read_html.
  • This one is using plotly.io.write_html while the other one is using plotly.io.write_json.

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

@yury-fedotov yury-fedotov changed the title Implement plotly.HTMLDataset feat: Implement plotly.HTMLDataset Jul 27, 2024
@yury-fedotov
Implement plotly.HTMLDataset
14d16d9 
Signed-off-by: Yury Fedotov <yury_fedotov@mckinsey.com>
@yury-fedotov yury-fedotov marked this pull request as ready for review July 27, 2024 02:33
@merelcht merelcht mentioned this pull request Aug 19, 2024
Closed
16 tasks
@merelcht
Update release notes
3653fee 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
merelcht
merelcht approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @yury-fedotov ! I'm happy to get this in as a regular dataset. It is indeed very similar to plotly.JSONDataset so I don't see a significant maintenance burden for the Kedro team to support this dataset.

We'll need 1/2 approval from the TSC to merge this dataset to the core datasets space. As of today we have 20 TSC members so we'll need 10 approvals in total.

@merelcht
Merge branch 'main' into 787-plotly-html-dataset
6388747 
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
astrojuanlu
astrojuanlu approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution @yury-fedotov ! 🙌🏼

@astrojuanlu astrojuanlu requested a review from a team August 21, 2024 14:29
ravi-kumar-pilla
ravi-kumar-pilla approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla 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 @yury-fedotov

tynandebold
tynandebold approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Excellent! Thank you!

DimedS
DimedS approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@DimedS DimedS left a comment

Choose a reason for hiding this comment

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

Thanks, @yury-fedotov !

@merelcht
Try fix windows docstring
affdf34 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
deepyaman
deepyaman approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

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

Going to approve, but have a few nit comments around alphabetization I'd appreciate. :D

kedro-datasets/docs/source/api/kedro_datasets.rst Show resolved Hide resolved
kedro-datasets/kedro_datasets/plotly/__init__.py Outdated Show resolved Hide resolved
kedro-datasets/pyproject.toml Outdated Show resolved Hide resolved
kedro-datasets/kedro_datasets/plotly/html_dataset.py Show resolved Hide resolved
merelcht and others added 3 commits August 21, 2024 17:08
@merelcht
Add encoding utf-8 as default fs arg
042823d 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht
Alphabetise
3640e8c 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht
Merge branch 'main' into 787-plotly-html-dataset
2e30282 
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
rashidakanchwala
rashidakanchwala approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

thanks @yury-fedotov

ElenaKhaustova
ElenaKhaustova approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova 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, @yury-fedotov!

@jitu5 jitu5 self-requested a review August 21, 2024 17:49
jitu5
jitu5 approved these changes Aug 21, 2024
View reviewed changes
Copy link

@jitu5 jitu5 left a comment

Choose a reason for hiding this comment

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

Thanks!

@merelcht
Copy link
Member

This has 12/20 TSC approval so I'm going to merge it. Thanks again for the contribution @yury-fedotov ⭐

@merelcht merelcht merged commit f9086a3 into kedro-org:main Aug 22, 2024
14 checks passed
merelcht pushed a commit to galenseilis/kedro-plugins that referenced this pull request Aug 27, 2024
@yury-fedotov @merelcht
feat: Implement plotly.HTMLDataset (kedro-org#788)
20f7ffc 
* Implement `plotly.HTMLDataset`

Signed-off-by: Yury Fedotov <yury_fedotov@mckinsey.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
harm-matthias-harms pushed a commit to harm-matthias-harms/kedro-plugins that referenced this pull request Oct 1, 2024
@yury-fedotov @harm-matthias-harms
feat: Implement plotly.HTMLDataset (kedro-org#788)
bbf1003 
* Implement `plotly.HTMLDataset`

Signed-off-by: Yury Fedotov <yury_fedotov@mckinsey.com>
Signed-off-by: Harm Matthias Harms <matthias.harms@quis.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElenaKhaustova ElenaKhaustova ElenaKhaustova approved these changes

@ankatiyar ankatiyar ankatiyar approved these changes

@ravi-kumar-pilla ravi-kumar-pilla ravi-kumar-pilla approved these changes

@merelcht merelcht merelcht approved these changes

@DimedS DimedS DimedS approved these changes

@jitu5 jitu5 jitu5 approved these changes

@rashidakanchwala rashidakanchwala rashidakanchwala approved these changes

@Galileo-Galilei Galileo-Galilei Galileo-Galilei approved these changes

@noklam noklam noklam approved these changes

@tynandebold tynandebold tynandebold approved these changes

@deepyaman deepyaman deepyaman approved these changes

@astrojuanlu astrojuanlu astrojuanlu approved these changes

@lrcouto lrcouto Awaiting requested review from lrcouto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kedro-datasets: Implement plotly.HTMLDataset