gh-95423: Updated winreg.DeleteKeyEx documentation for 32-bit OS. by derekdkim · Pull Request #95521 · python/cpython

derekdkim · 2022-08-01T05:39:31Z

Documentation updates to address:

Please let me know if my wording could use some improvement or if I missed any other part of the documentation.

Fixed some inaccuracies and added clarifications in the docstrings and documentation for winreg.DeleteKeyEx for how it behaves on 32-bit Windows.

ghost · 2022-08-01T05:39:33Z

All commit authors signed the Contributor License Agreement.

bedevere-bot · 2022-08-01T05:39:35Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

zooba · 2022-08-01T15:47:04Z

Thanks, it's a good start.

We don't need the note about NT 5.2 in here, because that applies to all of our supported platforms. We can also really simplify the note about 32-bit OS, since that's a vanishingly small number of machines these days.

The aim of our docs is to help Python developers use Python correctly. We don't need to provide historical or internal details of the platform, except where they are directly relevant to decisions that Python devs will have to make (and we are confident they won't change on us). Hopefully that helps guide your revisions.

# Conflicts: # Doc/library/winreg.rst # PC/winreg.c

derekdkim · 2022-08-01T17:02:30Z

Thanks for the feedback @zooba. I tried my best to cut down some of the bloat. Let me know if you think it can be simplified further.

Also, should I be committing the regenerated configure and topics.py? I wasn't sure what exactly needs to be done for the build step "Check if generated files are up to date" so I left it uncommitted for the time being.

zooba · 2022-08-01T21:32:15Z

Ah yeah, because you modified the docstrings, you need to run python Tools\clinic\clinic.py PC\winreg.c to update the generated code that includes them. Once you've done that, it should pass that check.

pythongh-95423: Regenerated winreg code with updated docstrings. pythongh-95423: Fixed default role backticks

zooba · 2022-08-02T15:56:54Z

For future, please don't force push once people have started reviewing. We always squash merge and edit commit messages, so no need to worry about that, and force pushes just break incremental reviews (i.e. I can't see what changes you just made, I have to start reading again).

derekdkim · 2022-08-02T16:06:02Z

Ah sorry, my bad. Noted.

zooba

I checked out the current state of the Reg function, and think we can drastically simplify. There aren't really any edge cases here anymore - it's just RegDeleteKeyExW all the time, and the WOW64 constants are ignored when they aren't relevant, so you can pass in anything you like.

zooba · 2022-08-02T16:03:57Z

   .. note::
      The :func:`DeleteKeyEx` function is implemented with the RegDeleteKeyEx
-      Windows API function, which is specific to 64-bit versions of Windows.
+      Windows API function, which is intended to be used for 64-bit Windows.
+      On 32-bit systems, it is strongly recommended to use :func:`DeleteKey()`
+      instead.


I did some more research, and it seems that there's no distinction on any modern Windows OS. RegDeleteKey just calls RegDeleteKeyEx directly and passes in zero for the extra parameter, so there isn't really any need for this note at all.

Got it, I removed the note completely.

zooba · 2022-08-02T16:05:13Z

+Deletes the specified key (intended for 64-bit OS).
+
+While this function is intended to be used for 64-bit OS, it is also
+ available on 32-bit systems.
+To use with 32-bit Windows, 0 must be passed in as the access argument.


Suggested change

Deletes the specified key (intended for 64-bit OS).

While this function is intended to be used for 64-bit OS, it is also

available on 32-bit systems.

To use with 32-bit Windows, 0 must be passed in as the access argument.

Deletes the specified key.

As above

bedevere-bot · 2022-08-02T16:07:07Z

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.

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

…c loading for RegDeleteKeyExW().

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

derekdkim · 2022-08-03T04:35:48Z

Thanks for the suggestions. I have made the requested changes; please review again. I think I was way too wordy and tunnel visioned on keeping the notes mentioned in the issue.

I also simplified winreg_DeleteKeyEx_impl() as per your suggestions. I removed the dynamic loading from the .dll and just called it directly similar to how winreg_DeleteKey_impl() is implemented. Let me know if I missed anything.

bedevere-bot · 2022-08-03T04:35:51Z

Thanks for making the requested changes!

@zooba: please review the changes made to this pull request.

zooba

With the one change (which I'll commit myself), this looks great!

derekdkim · 2022-08-03T17:19:14Z

@zooba Ah thanks for the note about the GIL around RegDeleteKeyExW(). Is there a reason why the same doesn't apply for RegDeleteKeyW() in winreg_DeleteKey_impl()?

zooba · 2022-08-03T19:30:43Z

Hmm... that seems like an oversight. Would you like to get it? I'm fine to not worry about it and just merge as it is.

derekdkim · 2022-08-03T19:51:44Z

No problem, I got it.

zooba · 2022-08-03T20:55:08Z

Awesome work, thanks!

miss-islington · 2022-08-03T20:55:47Z

Thanks @derekdkim for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington · 2022-08-03T20:55:47Z

Thanks @derekdkim for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2022-08-03T20:55:49Z

Sorry, @derekdkim and @zooba, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ebd660156d80bbb17899ca393731da5548100fc1 3.11

miss-islington · 2022-08-03T20:55:51Z

Sorry @derekdkim and @zooba, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ebd660156d80bbb17899ca393731da5548100fc1 3.10

bedevere-bot · 2022-08-03T21:01:01Z

GH-95619 is a backport of this pull request to the 3.11 branch.

…namic function load (pythonGH-95521)

bedevere-bot · 2022-08-03T21:14:36Z

GH-95622 is a backport of this pull request to the 3.10 branch.

…namic function load (pythonGH-95521)

…function load (GH-95521) Co-authored-by: Derek Kim <ddkim1024@gmail.com>

derekdkim added 2 commits August 1, 2022 00:30

pythongh-95423: Updated winreg.DeleteKeyEx documentation for 32-bit OS.

4c2e0f7

pythongh-95423: Updated winreg.DeleteKeyEx documentation for 32-bit OS.

4e87b5b

Fixed some inaccuracies and added clarifications in the docstrings and documentation for winreg.DeleteKeyEx for how it behaves on 32-bit Windows.

derekdkim requested a review from a team as a code owner August 1, 2022 05:39

bedevere-bot added the awaiting review label Aug 1, 2022

zooba added docs Documentation in the Doc dir skip news labels Aug 1, 2022

derekdkim added 2 commits August 1, 2022 11:53

pythongh-95423: Simplified the winreg.DeleteKeyEx doc note on 32-bit OS

215457a

Merge remote-tracking branch 'origin/pythongh-95423' into pythongh-95423

cf1ab9a

# Conflicts: # Doc/library/winreg.rst # PC/winreg.c

pythongh-95423: Fixes for CI issues

3536bd8

pythongh-95423: Regenerated winreg code with updated docstrings. pythongh-95423: Fixed default role backticks

derekdkim force-pushed the gh-95423 branch from a267b2f to 3536bd8 Compare August 2, 2022 02:41

Merge branch 'main' into pythongh-95423

e7d5146

zooba requested changes Aug 2, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Aug 2, 2022

bedevere-bot added the awaiting changes label Aug 2, 2022

derekdkim and others added 6 commits August 2, 2022 23:04

Update Doc/library/winreg.rst

bff044b

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

Update Doc/library/winreg.rst

8b532b0

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

Update PC/winreg.c

9f62a6d

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

pythongh-95423: Simplified winreg_DeleteKeyEx_impl(), removing dynami…

27e52d8

…c loading for RegDeleteKeyExW().

Update Doc/library/winreg.rst

4ad753c

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

pythongh-95423: Removed redundant note

659a185

bedevere-bot removed the awaiting changes label Aug 3, 2022

bedevere-bot added the awaiting change review label Aug 3, 2022

bedevere-bot requested a review from zooba August 3, 2022 04:35

zooba reviewed Aug 3, 2022

View reviewed changes

Comment thread PC/winreg.c

Update PC/winreg.c

03e3cb1

pythongh-95423: Added GIL handling for RegDeleteKeyW

7ee321a

zooba merged commit ebd6601 into python:main Aug 3, 2022

bedevere-bot removed the awaiting change review label Aug 3, 2022

zooba added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Aug 3, 2022

miss-islington assigned zooba Aug 3, 2022

bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 3, 2022

zooba pushed a commit to zooba/cpython that referenced this pull request Aug 3, 2022

pythongh-95423: Update winreg.DeleteKeyEx documentation and remove dy…

cda13e4

…namic function load (pythonGH-95521)

bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 3, 2022

zooba pushed a commit to zooba/cpython that referenced this pull request Aug 3, 2022

pythongh-95423: Update winreg.DeleteKeyEx documentation and remove dy…

4009d68

…namic function load (pythonGH-95521)

zooba added a commit that referenced this pull request Aug 3, 2022

gh-95423: Update winreg.DeleteKeyEx documentation and remove dynamic …

1016df0

…function load (GH-95521) Co-authored-by: Derek Kim <ddkim1024@gmail.com>

zooba added a commit that referenced this pull request Aug 3, 2022

gh-95423: Update winreg.DeleteKeyEx documentation and remove dynamic …

76d29a8

…function load (GH-95521) Co-authored-by: Derek Kim <ddkim1024@gmail.com>

derekdkim deleted the gh-95423 branch August 3, 2022 22:40

Uh oh!

Conversation

derekdkim commented Aug 1, 2022

Uh oh!

ghost commented Aug 1, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Aug 1, 2022

Uh oh!

zooba commented Aug 1, 2022

Uh oh!

derekdkim commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Aug 1, 2022

Uh oh!

zooba commented Aug 2, 2022

Uh oh!

derekdkim commented Aug 2, 2022

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zooba Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

derekdkim Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zooba Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Aug 2, 2022

Uh oh!

derekdkim commented Aug 3, 2022

Uh oh!

bedevere-bot commented Aug 3, 2022

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derekdkim commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Aug 3, 2022

Uh oh!

derekdkim commented Aug 3, 2022

Uh oh!

zooba commented Aug 3, 2022

Uh oh!

miss-islington commented Aug 3, 2022

Uh oh!

miss-islington commented Aug 3, 2022

Uh oh!

miss-islington commented Aug 3, 2022

Uh oh!

miss-islington commented Aug 3, 2022

Uh oh!

bedevere-bot commented Aug 3, 2022

Uh oh!

bedevere-bot commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Aug 1, 2022 •

edited by ghost

Loading

derekdkim commented Aug 1, 2022 •

edited

Loading

derekdkim Aug 3, 2022 •

edited

Loading

derekdkim commented Aug 3, 2022 •

edited

Loading