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

Unify cast_error message thrown by [simple|unpacking]_collector #3013

Merged
merged 2 commits into from
Jul 6, 2021

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented May 20, 2021

simple_collector and unpacking_collector throw different error messages when the casting of an argument failed:
While the former mentioned make_tuple(), the latter emphasizes the call argument (and its name/position):

[[noreturn]] static void argument_cast_error(std::string name, std::string type) {
throw cast_error("Unable to convert call argument '" + name
+ "' of type '" + type + "' to Python object");

This PR unifies both usages to use the latter (more informative) one.
If there was a reason to have different error messages for both collector variants, please feel free to close this PR.

Suggested changelog entry:

* Unify error messages thrown by ``simple_collector``/``unpacking_collector``.

@rhaschke
Copy link
Contributor Author

Closing and reopening to trigger CI.

@rhaschke rhaschke closed this May 20, 2021
@rhaschke rhaschke reopened this May 20, 2021
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I don't have historical background to know why the messages aren't uniform, but it seems likely to me that this simply didn't get enough attention and just slipped through the cracks. I think this is a good change to make. My only suggestion is to add simple unit tests for both, if possible actually one unit test that exercises both implementations, to ensure that the implementations are being kept consistent in the future.

rhaschke and others added 2 commits July 2, 2021 12:02
simple_collector and unpacking_collector throw different error messages
when the casting of an argument failed: While the former mentions make_tuple(),
the latter emphasises the call argument (and its name/position).
@rwgk
Copy link
Collaborator

rwgk commented Jul 2, 2021

I rebased this branch on current master and added a commit to consolidate the error reporting code. Waiting for the CI to finish.

@rwgk
Copy link
Collaborator

rwgk commented Jul 2, 2021

@Skylion007 Could you please take another look?

With respect to my comment from June 1: #3013 (review)

When looking at this PR again today, I decided it's better to ensure uniformity at the root (and reduce code duplication) rather than asserting something about the results (commit 53fc821).

I verified that the existing unit tests cover both call paths affected by the commit.

@rwgk rwgk added this to the v2.7 milestone Jul 3, 2021
@rwgk
Copy link
Collaborator

rwgk commented Jul 6, 2021

Thanks @rhaschke for initiating this PR, and thanks @Skylion007 for the review!

@rwgk rwgk merged commit c090c8c into pybind:master Jul 6, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 6, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
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.

4 participants