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

add as_sequence() method on dict views #2527

Closed
wants to merge 6 commits into from

Conversation

PrettyWood
Copy link
Contributor

@PrettyWood PrettyWood commented Aug 1, 2022

Hi and thank you for this amazing library! I'm trying to help @samuelcolvin on https://github.com/samuelcolvin/pydantic-core. Following #2358, it would be great to have at list a as_sequence() method on dict views to be able to easily get length, convert them to list, iterate...
I didn't want to add any other methods following @davidhewitt 's comment. Let's keep it simple for now :)

  • an entry in CHANGELOG.md
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

Copy link
Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@PrettyWood
Copy link
Contributor Author

I don't know what's wrong on windows and if it's even related to this PR 🤔

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks!

I don't know what's wrong on windows and if it's even related to this PR 🤔

Looks like The filename or extension is too long. (os error 206)...ugh. I don't know what's up with that.

src/types/dict.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
@PrettyWood
Copy link
Contributor Author

dankjewel @mejrs for the quick review! 🙏

@davidhewitt
Copy link
Member

Hey, thanks for this.

I'm definitely willing to add some methods to these views to improve usability, although I'm not convinced .as_sequence() is strictly correct. This appears to break type-safety, as I don't think these collections are sequences:

>>> import collections
>>> isinstance({}.keys(), collections.abc.Sequence)
False

You can already make use of the fact these views implement Deref<Target=PyAny> to use .len(), .iter() etc.

If we add methods to these types I would guess the main motivation would either be:

  • performance if there's a C API we can make use of
  • remove need for PyResult<_> on .len() etc.

Is it possible that someone could subtype dict_keys (e.g.) and create a __len__ which raises an exception? I think so, in which case we have to make a judgement call whether a bespoke fn len() -> usize on these types would panic in that case. So far in PyO3 we've made the decision to return results in all such cases so as to minimise library-level panics.

@PrettyWood
Copy link
Contributor Author

PrettyWood commented Aug 2, 2022

Ah you're absolutely right @davidhewitt! Thank you!
Since order is preserved, I thought dict views would be sequences but since they are not we can close this PR.


At first I also added a len() method indeed to avoid having a Result type but I understand the judgement call.

Thanks again for this library!

@PrettyWood PrettyWood closed this Aug 2, 2022
@PrettyWood PrettyWood deleted the dict-views-followup branch August 2, 2022 07:36
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.

None yet

4 participants