GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-30374: Fixed several bugs in win_add2path.py by XDcsy · Pull Request #1645 · python/cpython

Conversation

@XDcsy

@XDcsy XDcsy commented May 18, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
  • Discard DEFAULT = "%PATH%". The system PATH is effective to all users[1] so adding "%PATH%" in user PATH is unnecessary.
  • os.path.isdir() always return False because the input path includes "%APPDATA%" and also because the USER_SITE directory does not yet exist when Python is just installed. It is now deleted to make paths be added correctly.
  • The script compulsively sets the type of PATH var to REG_EXPAND_SZ to make the reference "%APPDATA%" effective[2] . However this is not a good practice (brought forward by Eryk Sun in the issue). Now the script does not change the type of PATH. If it's REG_SZ, the absolute path of %APPDATA% will be added instead.
  • The script needs users to log off and then log back on to take effect. Now it takes effect immediately by broadcasting a WM_SETTINGCHANG message[3] after the change of env vars.
  • Now displays error massages when fails to load PATH values or fails to refresh env vars.

https://bugs.python.org/issue30374

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@XDcsy
fix several bugs
ddaf7aa 
* Discard the DEFAULT "%PATH%" value.
* Delete wrong `os.path.isdir()`.
* Add absolute path instead of `%APPDATA%...` if the type of PATH is `REG_SZ`.
* Broadcast a `WM_SETTINGCHANG` message after the change of env vars.
* Add codes to deal with possible errors.
@mention-bot

mention-bot commented May 18, 2017

Copy link
Copy Markdown

@neEverett, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @tiran and @birkenfeld to be potential reviewers.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 18, 2017
@brettcannon brettcannon added the awaiting review label Feb 2, 2018
@serhiy-storchaka
serhiy-storchaka requested a review from a team December 8, 2018 10:31
@serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 8, 2018
zooba
zooba reviewed Feb 2, 2019
View reviewed changes

@zooba zooba left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for this! Overall they are good looking changes, just a few loose ends to tidy up.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/win_add2path.py Outdated
import site
import os
import winreg
import ctypes

@zooba zooba Feb 2, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Put in alphabetical order

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/win_add2path.py Outdated
envpath, dtype = winreg.QueryValueEx(key, PATH)
except FileNotFoundError:
envpath, dtype = "", winreg.REG_EXPAND_SZ
pass

@zooba zooba Feb 2, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary pass

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/win_add2path.py Outdated
except:
raise OSError("Failed to load PATH value")

if hasattr(site, "USER_SITE") and dtype == winreg.REG_EXPAND_SZ:

@zooba zooba Feb 2, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This hasattr check doesn't apply to the subsequent elif block, or rather, if the first check here fails then we may end up crashing in the elif block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
XDcsy added 2 commits February 4, 2019 11:42
@XDcsy
Tie up some loose ends
0cdb0e8 
Fixed the problems mentioned here: python#1645
@XDcsy
Tie up some loose ends
80e8874 
Fixed the problems mentioned here: python#1645
@csabella

csabella commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Closing and reopening to retrigger tests.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella csabella closed this Jan 27, 2020
@csabella csabella reopened this Jan 27, 2020
@csabella
csabella requested review from zooba and zware January 27, 2020 00:01
@github-actions

github-actions Bot commented Feb 19, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022
@github-actions

github-actions Bot commented Aug 28, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 28, 2022
@neEverett neEverett mannequin mentioned this pull request Nov 30, 2022
Make win_add2path.py take effect without having to log off #74559
Closed
@kumaraditya303

kumaraditya303 commented Nov 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Closing as win_add2path.py was removed in #98167. thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 closed this Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba Awaiting requested review from zooba
@zware zware Awaiting requested review from zware

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time. type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@XDcsy @mention-bot @csabella @kumaraditya303 @zooba @brettcannon @serhiy-storchaka @the-knights-who-say-ni @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.