Skip to content

Conversation

@tungol
Copy link
Contributor

@tungol tungol commented Nov 22, 2024

It should be a 6-tuple, once processor is added. This makes sure mypy understands that.

Fixes #13064

It should be a 6-tuple, once processor is added. This makes sure mypy understands that.

Fixes python#13064
@tungol tungol marked this pull request as draft November 22, 2024 07:01
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@tungol
Copy link
Contributor Author

tungol commented Nov 22, 2024

This seems to work as expected.

@tungol tungol marked this pull request as ready for review November 22, 2024 08:14
@github-actions

This comment has been minimized.

@tungol tungol closed this Nov 22, 2024
@tungol tungol reopened this Nov 22, 2024
@github-actions

This comment has been minimized.

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.

Wow, what a hacky (upstream) class ... I wonder whether it's a better idea to just use the else branch for both versions, considering that the hacks are probably meant to emulate a six-field namedtuple. This means that the special-casing of namedtuple in type checkers could work better. Although we probably need to override __init__ in that case ...

In any case, I think we should add test cases for this.

@tungol
Copy link
Contributor Author

tungol commented Nov 22, 2024

I couldn't figure out a way to re-create the exact failure from the daily test in the regression test. Is there any way to do a regression test stub right now? Is that something that might be useful for other cases as well?

The regression test shows that mypy accepts the tuple[str, str, str, str, str, str] but pyright is ignoring it, so we may need a different solution.

@github-actions

This comment has been minimized.

@tungol
Copy link
Contributor Author

tungol commented Nov 22, 2024

I opened a discussion topic for the difference between mypy and pyright in this case: https://discuss.python.org/t/conflicting-tuple-base-classes/72038

@tungol tungol marked this pull request as draft November 24, 2024 01:04
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@tungol tungol marked this pull request as ready for review November 24, 2024 11:22
@tungol
Copy link
Contributor Author

tungol commented Nov 24, 2024

I don't like how many ignores this needs. But it does satisfy both mypy and pyright, and should fix the daily test failure.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Why do we need the _uname_result_base class?

@tungol
Copy link
Contributor Author

tungol commented Nov 24, 2024

It's not adding much at this point. Here's a version without it.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this version seems cleaner

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@tungol
Copy link
Contributor Author

tungol commented Nov 25, 2024

This solution gets uname_result to be pretty accurate by pushing the inaccuracies into _uname_result_base. I think that's a good tradeoff to make; it's close to the runtime, clears all allowlist entries (because we don't test the hidden base class) and it only needs the one ignore.

@JelleZijlstra JelleZijlstra merged commit 410081e into python:main Nov 25, 2024
63 checks passed
@tungol tungol deleted the uname branch November 25, 2024 19:53
@AlexWaygood
Copy link
Member

Thanks @tungol!

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.

Daily tests failed on Fri Nov 22 2024

5 participants