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

refactor(typing): use abstract container types where possible #832

Merged
merged 4 commits into from Oct 6, 2022

Conversation

sisp
Copy link
Member

@sisp sisp commented Sep 30, 2022

I've replaced a few occurrences of concrete container types (Dict and List) by abstract container types (Mapping and Sequence) which is slightly better practice AFAIK. See, e.g.,

for an interesting article on this topic.

@sisp
Copy link
Member Author

sisp commented Sep 30, 2022

Hm, I'll need to check why the tests are failing for Linux and macOS. Surprisingly, they're passing for Windows. 🤔

@sisp sisp marked this pull request as draft September 30, 2022 19:00
@yajo
Copy link
Member

yajo commented Oct 3, 2022

I agree with the change, once the CI goes ✔️ .

Keep in mind that probably those annotations have a runtime effect with Pydantic. It might be the cause of the test problems.

@sisp
Copy link
Member Author

sisp commented Oct 4, 2022

I think you're right that the problem originates from Pydantic. Pydantic v1.10.2 handles the Mapping type incorrectly the way I see it:

from typing import Any, Dict, Mapping

from pydantic import parse_obj_as
from pydantic.dataclasses import dataclass


@dataclass
class Model:
    m: Mapping[Any, Any]
    d: Dict[Any, Any]


v = [["key", "value"]]
x = parse_obj_as(Model, {"m": v, "d": v})
print(x.m)  # ['key']
print(x.d)  # {'key': 'value'}

This behavior of Mapping vs. Dict is inconsistent and unexpected IMO.

@sisp sisp marked this pull request as ready for review October 4, 2022 12:08
@sisp
Copy link
Member Author

sisp commented Oct 4, 2022

I've reverted one usage of Mapping to Dict where Pydantic's behavior is odd. The tests are passing now; the one test that's failing is because of a Codecov upload issue.

@sisp
Copy link
Member Author

sisp commented Oct 5, 2022

I've created an issue over at Pydantic to indicate the Mapping bug: pydantic/pydantic#4588

@sisp
Copy link
Member Author

sisp commented Oct 5, 2022

I've submitted a PR to Pydantic which fixes the Mapping bug: pydantic/pydantic#4589

I suggest to proceed here without waiting for that PR to get merged because it's uncertain if and when it'll get merged and when a patch release for Pydantic will become available since they're working on Pydantic v2.

@sisp
Copy link
Member Author

sisp commented Oct 5, 2022

It seems this bug won't be fixed in Pydantic v1 and parsing a list of pairs as a dict seems to be a coincidental feature (see pydantic/pydantic#4588 (comment)).

@yajo
Copy link
Member

yajo commented Oct 6, 2022

the one test that's failing is because of a Codecov upload issue

Yet again... I can't believe how much often that fails... 🤦🏼‍♂️

@yajo yajo enabled auto-merge (rebase) October 6, 2022 12:08
@yajo yajo disabled auto-merge October 6, 2022 12:08
@yajo yajo enabled auto-merge (squash) October 6, 2022 12:08
@yajo yajo merged commit 482db41 into copier-org:master Oct 6, 2022
@sisp sisp deleted the refactor/use-abstract-container-types branch October 6, 2022 12:57
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

2 participants