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

Refactor ipywidgets to its own module and enhancements #697

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Refactor ipywidgets to its own module and enhancements #697

merged 5 commits into from
Oct 4, 2024

Conversation

ChristianZaccaria
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria commented Oct 2, 2024 •
edited
Loading

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-13330

What changes have been made

  • Refactor widgets to its own module
  • Converted UI table widgets to its own class.
  • Enhance widgets unit tests and increased coverage = 100%

Verification steps

  • e2e tests and UI notebooks tests should continue to pass.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2024
@ChristianZaccaria
Copy link
Collaborator Author

ChristianZaccaria commented Oct 2, 2024 •
edited
Loading

@KPostOffice In the latest commit here, I've converted each individual widget from the table to its own class. Let me know what you think, and if it meets with expectations, or if you had something else in mind wrt #681 (comment) Thank you!

KPostOffice
KPostOffice reviewed Oct 3, 2024
View reviewed changes
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Nice! I like it. Less args is nice. There's a failing unit test, but otherwise lgtm I think

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2024
@ChristianZaccaria ChristianZaccaria removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2024
@ChristianZaccaria ChristianZaccaria changed the title WIP: Refactor ipywidgets to its own module and enhancements Refactor ipywidgets to its own module and enhancements Oct 4, 2024
@ChristianZaccaria ChristianZaccaria force-pushed the refactor-ipywidgets branch 3 times, most recently from 0f14627 to 897f82d Compare October 4, 2024 10:44
@ChristianZaccaria ChristianZaccaria added the test-ui-notebooks Run PR check to verify UI notebooks label Oct 4, 2024
@ChristianZaccaria
Copy link
Collaborator Author

Rebased

Bobbins228
Bobbins228 approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Left a comment on changing the year of the copyright we can discuss some other time.
Overall this looks good and I appreciate the comment changes :)

/lgtm
/approve

tests/unit_test.py Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 4, 2024
Ygnas
Ygnas reviewed Oct 4, 2024
View reviewed changes
tests/unit_test.py Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2024
@Bobbins228
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 5cbe8a2 into project-codeflare:main Oct 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefanutti astefanutti Awaiting requested review from astefanutti

@Fiona-Waters Fiona-Waters Awaiting requested review from Fiona-Waters

@KPostOffice KPostOffice Awaiting requested review from KPostOffice

@Bobbins228 Bobbins228 Awaiting requested review from Bobbins228

@Ygnas Ygnas Awaiting requested review from Ygnas

Assignees

@Bobbins228 Bobbins228

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. test-ui-notebooks Run PR check to verify UI notebooks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ChristianZaccaria @Bobbins228 @Ygnas @KPostOffice @openshift-merge-robot