FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30374: Fixed several bugs in win_add2path.py by XDcsy · Pull Request #1645 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-30374: Fixed several bugs in win_add2path.py - #1645

Closed
XDcsy wants to merge 3 commits into
python:mainfrom
XDcsy:master
ConversationCommits (3)ChecksFiles changed
Closed

bpo-30374: Fixed several bugs in win_add2path.py#1645
XDcsy wants to merge 3 commits into
python:mainfrom
XDcsy:master

Conversation

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

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

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 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.

serhiy-storchaka requested a review from a team December 8, 2018 10:31
serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 8, 2018
zooba reviewed Feb 2, 2019
View reviewed changes

zooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Tools/scripts/win_add2path.py Outdated
import site
import os
import winreg
import ctypes

zooba Feb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Put in alphabetical order

Comment thread Tools/scripts/win_add2path.py Outdated
envpath, dtype = winreg.QueryValueEx(key, PATH)
except FileNotFoundError:
envpath, dtype = "", winreg.REG_EXPAND_SZ
pass

zooba Feb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unnecessary pass

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 Feb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

XDcsy added 2 commits February 4, 2019 11:42
Tie up some loose ends
0cdb0e8 
Fixed the problems mentioned here: python#1645
Tie up some loose ends
80e8874 
Fixed the problems mentioned here: python#1645

csabella commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Closing and reopening to retrigger tests.

csabella closed this Jan 27, 2020
csabella reopened this Jan 27, 2020
csabella requested review from zooba and zware January 27, 2020 00:01

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.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022

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.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 28, 2022
neEverett mannequin mentioned this pull request Nov 30, 2022
Closed

kumaraditya303 commented Nov 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

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.

9 participants

Back | FazBrowse Home | New Git URL