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

gh-123610: Added missing windows handle types to wintypes.py #124086

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Sep 14, 2024

I fixed the issue with missing handles that was mentioned in #123610 and added the missing handle types.

My wrong commit made it difficult to reset. This is my last branch: #123721

@bedevere-app
Copy link

bedevere-app bot commented Sep 14, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@rruuaanng
Copy link
Contributor Author

Hey @savannahostrowski, I’ve got a question I’d like to ask.
Based on the discussion in the PR #123721, That PR was last week, what should I do now?

@savannahostrowski
Copy link
Member

@rruuaanng Thanks for the PR. We'll need to wait for a core developer to review it. Sometimes, PRs can take a while to be reviewed since many do this on a volunteer basis. Let's leave this PR open and give folks time to review.

@picnixz
Copy link
Member

picnixz commented Sep 14, 2024

I'm the one who reviewed it last week. Since nothing seemed to have changed, I think this is fine. I had one interrogation namely, whether this warrants a NEWS entry and what granularity of details should be used.

I suggested NOT to add a NEWS entry since we didn't even bother adding a NEWS entry for the previous handle types which are not even documented. However, a core dev will need to confirm this and to agree on merging this PR. We could, in a follow-up PR, add some doc indicating which handles are available and which are not.

In the meantime, except waiting for such core dev, nothing more is needed. However, to spare CI resources, it would be preferrable not to merge the main branch into your branch unless you have merge conflicts to resolve.

EDIT: I'll add the skip news label just so that the PR appears "green" on the PR page.

@rruuaanng

This comment was marked as off-topic.

@rruuaanng

This comment was marked as off-topic.

@rruuaanng

This comment was marked as off-topic.

@erlend-aasland erlend-aasland requested a review from a team October 15, 2024 08:47
@zooba zooba merged commit 10c4c95 into python:main Oct 18, 2024
35 checks passed
@zooba
Copy link
Member

zooba commented Oct 18, 2024

Agreed we don't really need NEWS for this, on the basis that anyone already using these types must have worked around it in some way, and the workarounds won't be broken by us adding the types here. It can be a secret little easter egg for someone to find (hopefully we aren't encouraging people to use the deprecated APIs that used these types...)

@rruuaanng
Copy link
Contributor Author

Thanks to the reviewers for their efforts! Thanks for the merger of zooba! Maybe users will not use the above-mentioned abandoned API (to tell you quietly, Hmmmmm,they all actually used Linux).

@erlend-aasland
Copy link
Contributor

(hopefully we aren't encouraging people to use the deprecated APIs that used these types...)

I'm puzzled why we're adding (apparently) very seldom used and deprecated APIs.

@rruuaanng rruuaanng deleted the dev1 branch October 18, 2024 13:13
@zooba
Copy link
Member

zooba commented Oct 18, 2024

I'm puzzled why we're adding (apparently) very seldom used and deprecated APIs.

The APIs aren't in Python, they're Win32 APIs, and we aren't adding them specifically. Users still have to access them with ctypes.

These are just type aliases so that users who directly translate the C definitions into ctypes.wintypes will find the names there, and they've been checked. Some of these types aren't obvious that they aren't really void * sized, so we can reduce bugs in this way.

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants