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

feat: Standardize error message format in Rust code #11720

Closed
wants to merge 2 commits into from
Closed

feat: Standardize error message format in Rust code #11720

wants to merge 2 commits into from

Conversation

TheDataScientistNL
Copy link
Contributor

This code change updates all error messages in the rust part of polars package; see #10421

@stinodego stinodego changed the title style(rust): standardize error message format feat: Standardize error message format in Rust code Oct 14, 2023
@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Oct 14, 2023
@TheDataScientistNL TheDataScientistNL force-pushed the standardize_error_messages_rust branch from f208773 to 55c9f8f Compare October 14, 2023 17:45
@TheDataScientistNL TheDataScientistNL force-pushed the standardize_error_messages_rust branch from afca905 to c5f8aad Compare October 15, 2023 10:03
@TheDataScientistNL
Copy link
Contributor Author

@stinodego I have added many Rust error standardization corrections. Could you have a look? Not sure why the ubuntu 3.11 python tests fail. I have not, as far as I can see, changed anything that would result in the failing of a test with a null vs 0, and hesitant to push a fix....

@stinodego
Copy link
Contributor

That's an unrelated bug caught by our parametric tests - don't worry about it! I'll review this soon.

@orlp
Copy link
Collaborator

orlp commented Oct 16, 2023

This is a nitpick, but could you remove the periods from the end of error messages?

@stinodego
Copy link
Contributor

This is a nitpick, but could you remove the periods from the end of error messages?

Most don't have a period, right? The help text should have proper punctuation though. See #10421

@ritchie46
Copy link
Member

The single line messages shouldn't right?

@stinodego
Copy link
Contributor

The single line messages shouldn't right?

Correct. From the linked issue:

  • The error message starts with a single sentence, without capitalization or trailing period.
  • If further elaboration is required, two newlines are added before adding further text with proper capitalization and punctuation.

stinodego
stinodego suggested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@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.

A lot of improvements, but a lot of messages are now less clear because you stitched multi-sentence error messages together, instead of breaking them up into a short summary and a help text. Also, a lot of code snippets are still not in backticks.

This PR is too big for me to sit down and go through every example and correct the messages that I feel are regressions. Please break this up into multiple PRs, let's say one per crate, and really take care that the messages are high quality. Then we can actually get some of this work merged!

crates/polars-arrow/src/array/list/mod.rs
@@ -52,7 +52,7 @@ impl<O: Offset> ListArray<O> {
let child_data_type = Self::try_get_child(&data_type)?.data_type();
let values_data_type = values.data_type();
if child_data_type != values_data_type {
polars_bail!(ComputeError: "ListArray's child's DataType must match. However, the expected DataType is {child_data_type:?} while it got {values_data_type:?}.");
polars_bail!(ComputeError: "ListArray's child's DataType must match, yet the expected DataType is {child_data_type:?} while it got {values_data_type:?}");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too long for a single line error message. It should be broken up into a summary and a help text.

crates/polars-arrow/src/array/map/mod.rs
@@ -131,7 +131,7 @@ impl MapArray {
if let DataType::Map(field, _) = data_type.to_logical_type() {
Ok(field.as_ref())
} else {
polars_bail!(ComputeError: "The data_type's logical type must be DataType::Map")
polars_bail!(ComputeError: "the data_type's logical type must be DataType::Map")
Copy link
Contributor

Choose a reason for hiding this comment

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

Code snippets must be in backticks.

Suggested change
polars_bail!(ComputeError: "the data_type's logical type must be DataType::Map")
polars_bail!(ComputeError: "the data_type's logical type must be `DataType::Map`")

@TheDataScientistNL
Copy link
Contributor Author

TheDataScientistNL commented Oct 18, 2023 •
edited
Loading

A lot of improvements, but a lot of messages are now less clear because you stitched multi-sentence error messages together, instead of breaking them up into a short summary and a help text. Also, a lot of code snippets are still not in backticks.

This PR is too big for me to sit down and go through every example and correct the messages that I feel are regressions. Please break this up into multiple PRs, let's say one per crate, and really take care that the messages are high quality. Then we can actually get some of this work merged!

Will do. @stinodego Would you like a separate PR for each crate? Name of crate in PR name?

Also, what to do about package names? I see numpy, NumPy and several flavours around. Would you like standardization of that as well while we are at it? For instance, all packages/libraries used are lowercase, with tick marks?

@TheDataScientistNL TheDataScientistNL deleted the standardize_error_messages_rust branch October 18, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stinodego stinodego stinodego requested changes

@ritchie46 ritchie46 Awaiting requested review from ritchie46 ritchie46 is a code owner

@alexander-beedie alexander-beedie Awaiting requested review from alexander-beedie alexander-beedie is a code owner

@orlp orlp Awaiting requested review from orlp orlp is a code owner

@MarcoGorelli MarcoGorelli Awaiting requested review from MarcoGorelli MarcoGorelli is a code owner

@universalmind303 universalmind303 Awaiting requested review from universalmind303

@c-peters c-peters Awaiting requested review from c-peters c-peters is a code owner

Assignees
No one assigned
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheDataScientistNL @stinodego @orlp @ritchie46