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

Fix sequences that can be cast into PyMapping #2098

Closed
wants to merge 1 commit into from
Closed

Fix sequences that can be cast into PyMapping #2098

wants to merge 1 commit into from

Conversation

aviramha
Copy link
Member

Fixes #2072
Thanks @aganders3 for the finding the workaround.

@aviramha aviramha mentioned this pull request Jan 13, 2022
Closed
@aviramha aviramha force-pushed the py_mapping branch 2 times, most recently from ba0b92e to 20a1834 Compare January 13, 2022 17:16
@aviramha
Fix conversion of non-mapping sequences into PyMapping (i.e PyStr…
98bd0d3 
… can be cast to `PyMapping`) due to `PyMapping_Check` returning true for sequences.
@aviramha
Copy link
Member Author

I think tests are broken due to main being broken?

@aganders3
Copy link
Contributor

I think tests are broken due to main being broken?

Yes seems like it coincides with the 1.58 release. Mostly UI tests failing from what I see.

@mejrs
Copy link
Member

mejrs commented Jan 13, 2022

Yes, this is because of 1.58 I'll try to fix it ⚒️

@davidhewitt
Copy link
Member

davidhewitt commented Jan 13, 2022 •
edited
Loading

Hmmm, this is clever but I'm unsure it's the right fix.

As far as I know, classes implemented in Python with a __getitem__ will pass both PyMapping_Check and PySequence_Check, and so never be convertible to PyMapping with this change.

I was also proposing to match the Python behavior in #2065, which would mean PyO3 mappings would also fail this check.

Maybe we need to ask for help from upstream?

@aganders3
Copy link
Contributor

Ah, bummer.

As far as I know, classes implemented in Python with a getitem will pass both PyMapping_Check and PySequence_Check, and so never be convertible to PyMapping with this change.

Yeah I think this is true unless they also subclass dict.

@aviramha
Copy link
Member Author

Closing this, let's return to the original issue to discuss.

@aviramha aviramha closed this Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt Awaiting requested review from davidhewitt

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

Successfully merging this pull request may close these issues.

PyMapping is inconsistent with typing.Mapping
4 participants
@aviramha @aganders3 @mejrs @davidhewitt