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

Remove to_json from data objects and their associated tests #2626

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Remove to_json from data objects and their associated tests #2626

merged 3 commits into from
Dec 19, 2022

Conversation

mikejhuang
Copy link
Contributor

  • removed to_json method from data objects
  • removed JsonWritableInterface abstract base class and its references
  • removed tests that test to_json

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@mikejhuang mikejhuang changed the base branch from master to rc/2.16.1 December 7, 2022 21:29
morriscb
morriscb requested changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

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

Once the onprem test passes I'd say its ready to merge. Love deleting code.

@mikejhuang mikejhuang closed this Dec 8, 2022
@mikejhuang mikejhuang reopened this Dec 8, 2022
morriscb
morriscb approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

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

Didn't look like anything had changed since my previous review? Make sure to merge/rebase this on rc/2.14.1 instead of rc/2.16.1

@mikejhuang
Copy link
Contributor Author

Didn't look like anything had changed since my previous review? Make sure to merge/rebase this on rc/2.14.1 instead of rc/2.16.1

Your review didn't actually request changes other than to wait for the on-prem tests to finish and pass.

@mikejhuang mikejhuang changed the base branch from rc/2.16.1 to rc/2.14.1 December 14, 2022 20:03
@mikejhuang mikejhuang merged commit f923467 into rc/2.14.1 Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morriscb morriscb morriscb approved these changes

@aamster aamster Awaiting requested review from aamster

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@mikejhuang @morriscb