Skip to content

Conversation

@aisk
Copy link
Contributor

@aisk aisk commented Feb 9, 2025

Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Looks good, and I'd appreciate having the extra option there! Just need those audit events reverted - I don't think we need new ones.

@bedevere-app
Copy link

bedevere-app bot commented Feb 10, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Nearly there! Most important thing now I think is updating the real docs in Doc\library\winreg.rst.

An integer that specifies an access mask that describes the desired
security access for the key. Default is KEY_READ.
options: int = 0
Can be one of the REG_OPTION_* constants.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably document the interaction with reserved here. I'm not sure what the history is of reserved becoming a meaningful parameter, but it's meaningful now, so we should say that here (and in the real docs, Doc\library\winreg.rst). (I mean "here" as in a few lines above, but I can't comment on it because it hasn't been updated yet).

@aisk
Copy link
Contributor Author

aisk commented Mar 1, 2025

Hi, I just noticed that currently winreg.OpenKey is just an alias of winreg.OpenKeyEx instead of a direct wrapper of the Windows's RegOpenKey for some reason. Should we keep this, or just change it by don't add the new options parameter on it?

@zooba
Copy link
Member

zooba commented Mar 3, 2025

Hmm... perhaps we would be better to look at this as a stepping stone to replacing the reserved name with options in both cases? Something like this:

  • in 3.14, rename reserved to options and add reserved as keyword only
  • show a deprecation warning if reserved is non-zero (i.e. it was provided by name)
  • remove reserved in 3.16

And keep OpenKey the same. RegOpenKey is deprecated by the OS, so we want to be calling the new one (even though the old one likely won't ever go away, and is probably doing the same thing we are). It's already a direct alias, so no need to change anything.

@aisk aisk marked this pull request as draft March 12, 2025 14:21
@aisk aisk marked this pull request as ready for review March 12, 2025 15:53
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.

2 participants