-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-20523: pdb.Pdb can read the global ~/.pdbrc file on Windows 7 #11855
Conversation
Please also see open PR for the issue : #11847 |
@tirkarthi yep, thanks |
@tirkarthi what can I do? close my PR or add a test for my code. what do you think? |
I am not really sure about the correctness of either PRs. I just wanted to add the related open PR. |
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.
Works well
pass | ||
try: | ||
# bpo-20523: $HOME does not exist on Windows 7, | ||
# use os.path.expanduser() |
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 is just me being picky about comments, so use (or not) the following at your will:
1 - Although this has been vastly done in CPython, I'm personally against including the issue number (bpo-20523) at the code changes. This practice pollutes the de code and thinking forward, I wouldn't like to see a million bpo's
in the source code ten years from now.
2 - The second line use os.path.expanduser()
is redundant. A good comment should tell why
that change is necessary, and not how
you are accomplishing that. Your code should be readable enough for anyone to figure out the hows
. Which in this case, it's already readable enough.
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.
- we have 431 "bpo-XYZ: " in the code of CPython.
- I explain the reason of
os.path.expanduser
, because $HOME does not exist on Windows 7.
I would close this in favor of #11847, which is older and has a test. |
I close this PR because I have discussed with @tdhopper about his PR. |
https://bugs.python.org/issue20523