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

Merge TypedDicts from typeshed #4707

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Oct 28, 2024

Summary of changes

Follow-up to #4504, works towards #2345
I didn't include TypeDicts for **kwargs params as part of this PR. keyword args will be its own thing.

Pull Request Checklist

  • Changes have tests (existing type-checking and runtime tests)
  • News fragment added in newsfragments/. (not public facing yet)
    (See documentation for details)

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@@ -2038,14 +2038,19 @@ def chmod(path, mode):
log.debug("chmod failed: %s", e)


class SplitArgs(TypedDict, total=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we prefer introducing private terms?

Copy link
Contributor Author

@Avasam Avasam Oct 31, 2024

Choose a reason for hiding this comment

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

Is it intended for devs to assign to split_args at any point (and I mean re-assigning to the attribute directly, not mutating the dictionary)? If not, it could be made private.

The same reasoning can be applied to TypeDict used solely in args vs return types. To let users instantiate/reference the TypedDict w/o importing a private symbol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally users should not be importing more symbols in general 😅, and more so from deprecated modules.

Hopefully devs will not have to assign new values, but if they do I think they should simply use a literal dict with the correct keys. Mypy should be able to cope with that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes using a dict literal to assign to a typeddict should be fine. The module being deprecated is even more reason. I'll make them both private.
If a user really needs to they can still import the private symbol for reference.

@Avasam Avasam force-pushed the Merge-TypedDict-from-typeshed branch 4 times, most recently from df16933 to 0353c55 Compare November 1, 2024 18:15
@Avasam Avasam force-pushed the Merge-TypedDict-from-typeshed branch from 0353c55 to eca1d66 Compare November 1, 2024 18:17
@abravalheri abravalheri merged commit 12aadb1 into pypa:main Nov 4, 2024
22 of 24 checks passed
@Avasam Avasam deleted the Merge-TypedDict-from-typeshed branch November 4, 2024 17:21
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.

2 participants