Skip to content

Add some missing attribute annotations to trace.pyi #11091

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
Dec 3, 2023

Conversation

AlexWaygood
Copy link
Member

This will be useful for python/cpython#109413

@AlexWaygood AlexWaygood reopened this Nov 30, 2023

This comment has been minimized.

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

I have two questions, other than that, this looks good assuming __init__ was already typed correctly*.

  1. (see in-code comment about TraceFunction)
  2. *infile and outfile look like they could work as file descriptors. The online documentation says "name of the file" specifically, but docstring only says "file" and stub also already accepts Path. I know for the stdlib typeshed sometimes differs from "what's possible" to "do the correct thing", so I'll defer to your judgement here.

Copy link
Contributor

github-actions bot commented Dec 3, 2023

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

@AlexWaygood
Copy link
Member Author

Thanks, I made the suggested change regarding localtrace and globaltrace!

2. *infile and outfile look like they could work as file descriptors.

Ehhhh, not sure. In general, I kinda feel like passing file descriptors directly into high-level functions like this is bad practice, and usually only works by accident (rather than the core devs deliberately designing functions in the trace module so that they'll accept file descriptors). My personal feeling is I'm generally happy to consider adding file-descriptor support to stubs if someone asks for it, but otherwise, I'd lean against doing so unless the function is explicitly documented as supporting file descriptors (or it's clear from the git history that file-descriptor support is deliberate, etc.).

@AlexWaygood AlexWaygood requested a review from Avasam December 3, 2023 22:11
@AlexWaygood AlexWaygood merged commit 77ff987 into python:main Dec 3, 2023
@AlexWaygood AlexWaygood deleted the trace-attrs branch December 3, 2023 22:14
@AlexWaygood
Copy link
Member Author

Thanks for the review! :D

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