Skip to content

Conversation

@charlesbluca
Copy link
Collaborator

As a follow up on reviews I received in #656, this PR replaces the panic calls within Python-facing methods in dask_planner and replaces them with Err(PyErr)), making modifications to signatures and logic where necessary to get this working.

This PR also adds/modifies the custom exceptions so that they can be used throughout the codebase instead of PyErr::new.

@charlesbluca charlesbluca changed the base branch from main to datafusion-sql-planner August 4, 2022 20:28
@jdye64
Copy link
Collaborator

jdye64 commented Aug 4, 2022

This would also resolve #510

@charlesbluca charlesbluca linked an issue Aug 4, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (datafusion-sql-planner@f948429). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             datafusion-sql-planner     #662   +/-   ##
=========================================================
  Coverage                          ?   65.70%           
=========================================================
  Files                             ?       73           
  Lines                             ?     3636           
  Branches                          ?      754           
=========================================================
  Hits                              ?     2389           
  Misses                            ?     1098           
  Partials                          ?      149           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@jdye64 jdye64 left a comment

Choose a reason for hiding this comment

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

This has been long due. Thanks for taking it on

Copy link
Collaborator

@jdye64 jdye64 left a comment

Choose a reason for hiding this comment

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

Long overdue and much appreciated!

@jdye64
Copy link
Collaborator

jdye64 commented Aug 8, 2022

I'm going to merge this since the failures are simply codecov failing to upload. Thanks again @charlesbluca

@jdye64 jdye64 merged commit 24afec8 into dask-contrib:datafusion-sql-planner Aug 8, 2022
@charlesbluca
Copy link
Collaborator Author

Thanks @jdye64! Yeah for context I'm currently working on #666 to make it so Codecov uploads won't block CI

@charlesbluca charlesbluca deleted the rust-error-string-cleanup branch August 31, 2022 13:10
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.

[DF] Replace panic! instances with PyErr(s)

3 participants