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.

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

513 Deprecating Legacy Plot API #870

Merged
merged 12 commits into from
Sep 14, 2023
Merged

513 Deprecating Legacy Plot API #870

merged 12 commits into from
Sep 14, 2023

Conversation

bryannho
Copy link

@bryannho bryannho commented Sep 7, 2023

Describe your changes

  • Added deprecation warnings for legacy plot functions: .pie(), .bar(), and .plot()
  • Added tests for deprecation warning messages
  • Merged from master instead of individual branch

Issue number

Closes #513

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--870.org.readthedocs.build/en/870/

@bryannho bryannho marked this pull request as ready for review September 7, 2023 19:14
@edublancas edublancas mentioned this pull request Sep 7, 2023
4 tasks
Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

please ensure tests are passing before requesting a review

@bryannho bryannho reopened this Sep 9, 2023
@bryannho
Copy link
Author

I was able to identify and fix the matplotlib bug. The tests I wrote in test_resultset.py were interfering with those in test_util.py. After re-writing my tests all checks are passed.

src/sql/plot.py Outdated Show resolved Hide resolved
src/tests/test_resultset.py Outdated Show resolved Hide resolved
src/tests/test_resultset.py Outdated Show resolved Hide resolved
src/tests/test_resultset.py Show resolved Hide resolved
src/tests/test_resultset.py Outdated Show resolved Hide resolved
@edublancas
Copy link

@bryannho no need to respond to the PR comments when you begin working on stuff. it's fine if you respond to them after you finished the changes

Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

please review our contribution guidelines: https://ploomber-contributing.readthedocs.io/en/latest/contributing/responding-pr-review.html

when responding to PR comments, you need to add a link to the specific changes, so the reviewer can quickly spot them. add the corresponding links and request a review again

@edublancas edublancas merged commit 202f043 into ploomber:master Sep 14, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deprecating legacy plot API
2 participants