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

Set difference strictly #3886

Merged
merged 1 commit into from Sep 17, 2020
Merged

Set difference strictly #3886

merged 1 commit into from Sep 17, 2020

Conversation

proost
Copy link
Contributor

@proost proost commented Mar 28, 2020

close #1840

I think sub operator too.
Because "frozenset" also strictly compare.
It makes sense for consistency.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I just ran this against our internal code base and it flagged no problems. This is still a bit risky, so I'd prefer one other review. (@JelleZijlstra @ilevkivskyi @JukkaL)

@JukkaL
Copy link
Contributor

JukkaL commented Apr 3, 2020

This can generate false positives, since the set item types arguably only need to be overlapping, not compatible. I wouldn't be surprised if this would generate issues with our internal codebases (but I haven't verified yet). I'll run this against our internal codebases next week (if I remember to :-).

@srittau
Copy link
Collaborator

srittau commented May 27, 2020

@JukkaL Did you remember? :)

@JukkaL
Copy link
Contributor

JukkaL commented May 27, 2020

I've been too distracted. I'll try to find some time to try this out later this week.

@srittau
Copy link
Collaborator

srittau commented Sep 17, 2020

I am going ahead and merge this. If this causes too many problems down the line, we need to revert it.

@srittau srittau merged commit b186563 into python:master Sep 17, 2020
@proost proost deleted the set-sub-operator branch September 19, 2020 07:56
@JelleZijlstra
Copy link
Member

@hauntsaninja ran mypy-primer on recent master (python/mypy#9290) and one new false positive seems to come from this change:
+ homeassistant/helpers/device_registry.py:568: error: Unsupported operand types for - ("Set[str]" and "Set[Optional[str]]")

@srittau
Copy link
Collaborator

srittau commented Oct 1, 2020

@JelleZijlstra Is that really a false positive, though? While we could add a special exception for None, the right-hand side set containing a None hints at a potential problem to me. Of course, it will work at runtime, but so will Set[str] - Set[int].

@JelleZijlstra
Copy link
Member

Honestly the code (which is at https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/device_registry.py#L568) doesn't seem particularly problematic to me. It's a different story if you have completely non-overlapping types like str and int, but Set[str] - Set[Optional[str]] doesn't seem like an unreasonable thing to do.

@JelleZijlstra
Copy link
Member

Then again, maybe this case is rare enough that the change is still worth it; we'll have to see how often this comes up in practice.

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.

Set difference unexpectedly permissive
4 participants