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

Left-hand operands are fellback to right-hand ones for type mismatching #1107

Merged
merged 5 commits into from
Aug 21, 2020
Merged

Left-hand operands are fellback to right-hand ones for type mismatching #1107

merged 5 commits into from
Aug 21, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Aug 15, 2020 •
edited
Loading

Fixes #844.
Include some other refactorings:

  • SlotSetter is refactored not to have skipped_setters field.
  • py_ternary_num_func macros are removed since they are just called once.
  • #[macro_export] for macros in class/macros.rs are removed.

@kngwyu kngwyu changed the title Left-hand operands are fallbacked to right-hand ones for type mismatching Left-hand operands are fellback to right-hand ones for type mismatching Aug 16, 2020
@kngwyu
Copy link
Member Author

kngwyu commented Aug 16, 2020

And I changed right hand methods to use extract_or_return_not_implemented, so now we can write tests that confirm that NotImplemented is correctly returned when both of __add__ and __radd__ is defined. cc: @mvaled

@kngwyu
Copy link
Member Author

kngwyu commented Aug 16, 2020

Sigh, coverage decreased after I added some tests... 😓

davidhewitt
davidhewitt reviewed Aug 17, 2020
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

🚀 thanks, looks great to me! Just got a few suggestions to tidy things up a little.

Regarding the coverage, yeah that's a little annoying isn't it. I wonder if we can configure codecov to ignore changes < 1% ? Or maybe it's possible to figure out why codecov thinks the coverage has changed. Either way, it's not a problem and we can merge anyway (after making changes for any suggestions you like) 😄

Also, needs a CHANGELOG entry 😉

pyo3-derive-backend/src/pyproto.rs Outdated Show resolved Hide resolved
pyo3-derive-backend/src/pyproto.rs Outdated Show resolved Hide resolved
tests/test_arithmetics.rs Outdated Show resolved Hide resolved
@kngwyu kngwyu force-pushed the radd-fallback branch 2 times, most recently from acc48b8 to e44c85b Compare August 18, 2020 08:11
davidhewitt
davidhewitt approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This looks really great! Feel free to take on or ignore any of these minor ideas as you like before merging 😄

CHANGELOG.md Outdated Show resolved Hide resolved
pyo3-derive-backend/src/pyproto.rs Show resolved Hide resolved
src/class/macros.rs Outdated Show resolved Hide resolved
tests/test_arithmetics.rs Show resolved Hide resolved
tests/test_arithmetics.rs Show resolved Hide resolved
pyo3-derive-backend/src/defs.rs Outdated Show resolved Hide resolved
davidhewitt
davidhewitt approved these changes Aug 20, 2020
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

💯

pyo3-derive-backend/src/defs.rs
&self,
mut implemented_protocols: HashSet<String>,
) -> impl Iterator<Item = &'static str> {
self.slot_setters.iter().filter_map(move |setter| {
Copy link
Member

Choose a reason for hiding this comment

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

Nice one, this implementation is really elegant!

@kngwyu kngwyu merged commit 9d73e0b into PyO3:master Aug 21, 2020
@kngwyu kngwyu deleted the radd-fallback branch August 21, 2020 08:10
ravenexp added a commit to ravenexp/pyo3 that referenced this pull request Apr 5, 2021
@ravenexp
Remove issue PyO3#844 mention
5a886d9 
Supposedly resolved by PyO3#1107
ravenexp added a commit to ravenexp/pyo3 that referenced this pull request Apr 5, 2021
@ravenexp
Remove issue PyO3#844 mention from the guide
0c02146 
Supposedly resolved by PyO3#1107

Fix a typo in the subsection header.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

__radd__ etc don't get called if __add__ is also defined
2 participants
@kngwyu @davidhewitt