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

feat(python)!: Improve some error types and messages #10470

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

aminalaee
Copy link
Contributor

@aminalaee aminalaee commented Aug 14, 2023

Partially addresses #10421

Changes:

  • Improve error messages to be in accordance with the guidelines (see linked issue).
  • Fix the error types of some functions. This could be considered a breaking change.

Copy link
Member

@stinodego stinodego 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 effort! Though I definitely see some issues still - messages that start with a capital letter, or mistakenly added newlines.

I'll put this on draft, just mark it ready-to-review when you've made a pass to verify your changes!

@stinodego stinodego marked this pull request as draft August 14, 2023 12:21
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Aug 14, 2023

Nice one! I was going to look at this but you're off to a great start - go for it :))

@mcrumiller
Copy link
Contributor

Out of curiosity, would we benefit from an error-helper function, or is this too much indirection? Something like

pl.exceptions.raise(
    "ValueError",
    header="polars can not currently guarantee zero-copy conversion from Arrow for categorical columns",
    desc="Set `allow_copy=True` or cast categorical columns to string first."
)

Which could then ensure proper formatting, and insert newlines where appropriate?

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Aug 14, 2023

Out of curiosity, would we benefit from an error-helper function, or is this too much indirection? Something like

Perhaps, but it would find its way into the reported stack which isn't ideal 🤔

@mcrumiller
Copy link
Contributor

@alexander-beedie there are ways to modify the traceback with the traceback module, Jinja is well-known for doing this. But it's probably more effort than it's worth, especially if we want to be compatible on multiple versions.

@alexander-beedie
Copy link
Collaborator

@alexander-beedie there are ways to modify the traceback with the traceback module, Jinja is well-known for doing this. But it's probably more effort than it's worth, especially if we want to be compatible on multiple versions.

Indeed; doing "clever" things with your exceptions IN your exception handling code is something I'm not a big fan of ;)

@aminalaee aminalaee force-pushed the standardize-error-messages branch 4 times, most recently from 670cff9 to 761b1f4 Compare August 15, 2023 07:59
@aminalaee aminalaee marked this pull request as ready for review August 15, 2023 08:35
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Nice improvement! Congrats on your first contribution 🎉

I did have to fix a few false positives, and there are many messages that still do not conform to the guidelines. Plus the same improvements should be done on the Rust side. So the linked issue remains open for now.

If you wish to continue to work on the issue, I would advise a more concentrated approach: pick a module and make sure all error messages are perfect. That way, it's easier for us to review.

@stinodego stinodego changed the title Standardize error message format feat(python): Standardize error message format Aug 15, 2023
@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars labels Aug 15, 2023
@stinodego stinodego changed the title feat(python): Standardize error message format feat(python)!: Standardize error message format Aug 15, 2023
@github-actions github-actions bot added the breaking Change that breaks backwards compatibility label Aug 15, 2023
@stinodego stinodego changed the title feat(python)!: Standardize error message format feat(python)!: Improve error types and messages Aug 15, 2023
@stinodego stinodego changed the title feat(python)!: Improve error types and messages feat(python)!: Improve some error types and messages Aug 15, 2023
@stinodego stinodego merged commit 2bc2de6 into pola-rs:main Aug 16, 2023
18 checks passed
@mcrumiller
Copy link
Contributor

@aminalaee 55 files changed! Good job here. We all encounter error messages every day so this is a nice improvement :)

@aminalaee aminalaee deleted the standardize-error-messages branch August 16, 2023 05:02
@aminalaee
Copy link
Contributor Author

Thanks for the review and kind words.
Yes I would love to keep working on the issue on both Python and Rust side and as you said it is better to go module by module as there are cases that are harder to find. For example I saw this by going through the files:

msg =Invalid operationraise ValueError(msg)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants