Skip to content

csv.Dialect doesn't actually subclass _csv.Dialect #12808

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

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Oct 14, 2024

This comment has been minimized.

@JelleZijlstra
Copy link
Member

See #3613 for some prior history.

@tungol tungol marked this pull request as draft October 14, 2024 22:51
@tungol
Copy link
Contributor Author

tungol commented Oct 14, 2024

Switched to draft while I look into the mypy-primer result and the prior history that Jelle linked.

@tungol tungol marked this pull request as ready for review October 14, 2024 23:07
@tungol
Copy link
Contributor Author

tungol commented Oct 14, 2024

In the prior history, it was settled to have only one class, _csv.Dialect, and to re-export that in csv. Since then, #12320 re-added a distinct csv.Dialect class in order that both csv.Dialect and _csv.Dialect would have correct __init__ methods.

If I add csv.Dialect to the _DialectLike alias introduced by #12320 , I believe that should resolve the issue with correct inheritance structure.

Copy link
Contributor

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

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.

LGTM, has no primer impact and is closer to the implementation. But I have no strong feelings about this.

@JelleZijlstra JelleZijlstra merged commit 56078f5 into python:main Oct 17, 2024
63 checks passed
@tungol tungol deleted the csv branch October 17, 2024 22:38
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.

3 participants