Skip to content

bpo-20523: pdb.Pdb can read the global ~/.pdbrc file on Windows 7#11855

Closed
matrixise wants to merge 1 commit intopython:masterfrom
matrixise:bpo-20523
Closed

bpo-20523: pdb.Pdb can read the global ~/.pdbrc file on Windows 7#11855
matrixise wants to merge 1 commit intopython:masterfrom
matrixise:bpo-20523

Conversation

@matrixise
Copy link
Copy Markdown
Member

@matrixise matrixise commented Feb 14, 2019

@tirkarthi
Copy link
Copy Markdown
Member

Please also see open PR for the issue : #11847

@matrixise
Copy link
Copy Markdown
Member Author

@tirkarthi yep, thanks

@matrixise
Copy link
Copy Markdown
Member Author

@tirkarthi what can I do? close my PR or add a test for my code. what do you think?

@tirkarthi
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown

@Ishaan28malik Ishaan28malik left a comment

Choose a reason for hiding this comment

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

Works well

Comment thread Lib/pdb.py
pass
try:
# bpo-20523: $HOME does not exist on Windows 7,
# use os.path.expanduser()
Copy link
Copy Markdown
Contributor

@augustogoulart augustogoulart Feb 15, 2019

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@augustogoulart

  1. we have 431 "bpo-XYZ: " in the code of CPython.
  2. I explain the reason of os.path.expanduser, because $HOME does not exist on Windows 7.

@blueyed
Copy link
Copy Markdown
Contributor

blueyed commented Feb 15, 2019

I would close this in favor of #11847, which is older and has a test.
AFAIK code was changed according to feedback from @matrixise, so should be contributed there maybe.

@matrixise
Copy link
Copy Markdown
Member Author

I close this PR because I have discussed with @tdhopper about his PR.

@matrixise matrixise closed this Feb 18, 2019
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.

7 participants