-
Notifications
You must be signed in to change notification settings - Fork 77
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
Plot error message #441
Plot error message #441
Conversation
This is ready for review. Some notes:
So directly used the |
For 4:
Take a look in the core repo, there's the tag we've added, it'll help you and you won't need to deal with the community link manually. |
Yeah I tried that out like:
But the issue with that is , first it prints the msg
|
42e0315
to
f2b2cbb
Compare
Modified to raise |
I think it makes more sense to use a |
We can use the existing RuntimeError. I don't think we should be that specific to create a PlotError. And UsageError isn't the appropriate one here since that should only be used when the user input is the problem. Here the problem is the database (example: doesn't implement the percentile_disc function) |
Changed |
589f5de
to
bd0a510
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some notes
@neelasha23
assert ( | ||
"If you need help solving this issue, " | ||
"send us a message: https://ploomber.io/community" in str(out.error_in_exec) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should add a successful test where we use percentile_disc without getting this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the success testcase:
def test_sqlplot_boxplot(ip_with_dynamic_db, cell, request, test_table_name_dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought maybe we can create a scoped test to make sure that _summary_stats
runs fluently with percentile_disc
since test_sqlplot_boxplot
may fail for other reasons. I guess we can skip it.
@idomic what do you think?
if not, we can merge it from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neelasha23 I tend to agree with Yafim, this test should be isolated. Let’s create a clean one just for the success case of _summary_stats
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit cases for _summary_stats
25f713d
to
4eb2d73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
I added one question before we merge it.
8dacd02
to
76c83fe
Compare
testcase Fix Fix Fix test fix test fix test fix changelog usageexception test test error error error error error changed to runtimeerror quest db test Revert test Revert test exception type test Review comments changelog rebase changelog Review comments changed to sqlite change sequence lint revert added tmp_empty failure case
lgtm. |
Describe your changes
Better error messages when function used in plotting API is unavailable in the driver.
Issue number
Closes #159
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--441.org.readthedocs.build/en/441/