FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove pathlib.PurePath.__eq__ by hauntsaninja · Pull Request #10662 · python/typeshed · GitHub

Repository navigation

Remove pathlib.PurePath.__eq__ - #10662

Merged
hauntsaninja merged 4 commits into
python:mainfrom
hauntsaninja:pathlib-strict-eq
Sep 7, 2023
Merged

hauntsaninja merged 4 commits into
python:mainfrom
hauntsaninja:pathlib-strict-eq

Conversation

Copy link
Copy Markdown
Collaborator

Fixes #10661

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

I think the primer hits here are real issues

This comment has been minimized.

hauntsaninja marked this pull request as ready for review September 4, 2023 23:58
Comment thread test_cases/stdlib/check_pathlib.py Outdated
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It still feels like we're sorta working around shortcomings in the heuristics mypy/pyright use here. (As you said in the issue, this kind of thing is going to affect anybody who writes a __eq__ method in a py.typed runtime library, and __eq__ methods are pretty common!)

But I agree that this is a pretty useful error for mypy/pyright to emit, so I'm happy for this change to be made on the basis that practicality beats purity :-)

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Sounds good, I'll leave open for a bit in case anyone else chimes in!

AlexWaygood commented Sep 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Here is a small regression that this PR causes, FYI: currently mypy will not complain about this code (on my Windows machine, the expression evaluates to True):

from pathlib import *
PureWindowsPath("foo/bar") == Path("foo/bar")

But with this PR, it will:

$ mypy -c "from pathlib import *; PureWindowsPath('foo/bar') == Path('foo/bar')" --strict-equality --custom-typeshed-dir .
<string>:1: error: Non-overlapping equality check (left operand type: "PureWindowsPath", right operand type: "Path")  [comparison-overlap]
Found 1 error in 1 file (checked 1 source file)

Is the regression that important? Probably not particularly; I don't know how many people are doing this kind of comparison. But, worth noting, I think.

hauntsaninja commented Sep 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@AlexWaygood oh, interesting example! Definitely a false positive in terms of what the type checker thinks it's doing, but that code is pretty suspect and I'm okay with it triggering a warning of some kind — depending on your platform, it could be true that those could never be equal!

So I think I'm still fine with the PR (though I'll certainly add test case and comments). We should experiment with making Path.__new__ more accurate in a follow up. I'm trying to teach mypy about more kinds of __new__ over here: python/mypy#16020

github-actions Bot commented Sep 5, 2023

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

mkosi (https://github.com/systemd/mkosi)
+ mkosi/kmod.py:32:16: error: Non-overlapping container check (element type: "str", container item type: "Path")  [comparison-overlap]
+ mkosi/config.py:2012:8: error: Non-overlapping equality check (left operand type: "Optional[Path]", right operand type: "Literal['']")  [comparison-overlap]

hauntsaninja merged commit 4df9634 into python:main Sep 7, 2023
hauntsaninja deleted the pathlib-strict-eq branch September 7, 2023 07:40

AlexWaygood commented Sep 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This change caught a real bug in stubtest (that was introduced by, er, me)! https://github.com/python/mypy/actions/runs/6202155467/job/16840319289?pr=16121

The first CI run on python/mypy#16121

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

comparison-overlap not triggered for pathlib.Path

3 participants


Back | FazBrowse Home | New Git URL