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-28334: fix netrc not working when $HOME is not set by dmerejkowsky · Pull Request #123 · python/cpython

Conversation

@dmerejkowsky

@dmerejkowsky dmerejkowsky commented Feb 15, 2017 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown

https://bugs.python.org/issue28334

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
guyzmo and yan12125 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@the-knights-who-say-ni

the-knights-who-say-ni commented Feb 15, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!
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 not signed label Feb 15, 2017
@dmerejkowsky

dmerejkowsky commented Feb 15, 2017

Copy link
Copy Markdown
Author

As discussed during patch review, no tests were added, but doc was updated.
All reactions

Sorry, something went wrong.

Uh oh!

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

dmerejkowsky commented Feb 15, 2017

Copy link
Copy Markdown
Author

@the-knights-who-say-ni :
Added my github username on bugs.python.org as requested.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta removed the CLA not signed label Feb 15, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 15, 2017
@dmerejkowsky dmerejkowsky mentioned this pull request Feb 18, 2017
bpo-28806: Improve the netrc library #127
Closed
berkerpeksag
berkerpeksag requested changes Feb 19, 2017
View reviewed changes

@berkerpeksag berkerpeksag 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 the PR, but like I said in review we can't accept this without a test case.

It should be easy to test at least the FileNotFoundError case by changing $HOME env variable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/netrc.rst Outdated

@berkerpeksag berkerpeksag Feb 19, 2017

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.

Style nit:

.. versionchanged:: 3.7
   Now uses :func:`os.path.expanduser`, so if [...]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch from f9e303b to cb9777b Compare February 20, 2017 19:14
@dmerejkowsky

dmerejkowsky commented Feb 20, 2017

Copy link
Copy Markdown
Author

Rebased, added a test, and fix doc style as requested.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zhangyangyu
zhangyangyu self-requested a review February 22, 2017 09:55
zhangyangyu
zhangyangyu reviewed Mar 8, 2017
View reviewed changes
Comment thread Lib/test/test_netrc.py Outdated

@zhangyangyu zhangyangyu Mar 8, 2017

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.

I would like the name just test_file_not_found.

Sorry, something went wrong.

Uh oh!

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

@dmerejkowsky dmerejkowsky Mar 11, 2017

Copy link
Copy Markdown
Author

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.

agreed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
zhangyangyu
zhangyangyu reviewed Mar 8, 2017
View reviewed changes
Comment thread Lib/test/test_netrc.py Outdated

@zhangyangyu zhangyangyu Mar 8, 2017

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.

When you explicitly pass a not exist file, it could also raise the error.

Sorry, something went wrong.

Uh oh!

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

@dmerejkowsky dmerejkowsky Mar 11, 2017

Copy link
Copy Markdown
Author

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.

OK. I wrote an other test for this. (I like to follow the 'one assert per test' rule)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@zhangyangyu
zhangyangyu requested a review from terryjreedy March 8, 2017 09:29
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch from cb9777b to 7119217 Compare March 11, 2017 18:56
zhangyangyu
zhangyangyu approved these changes Mar 13, 2017
View reviewed changes
@zhangyangyu

zhangyangyu commented Mar 13, 2017

Copy link
Copy Markdown
Member

LGTM. But I'd wait for others' reviews for some time. And I want to treat this as an enhancement though it looks like a bug on Windows.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
berkerpeksag
berkerpeksag requested changes Mar 15, 2017
View reviewed changes

@berkerpeksag berkerpeksag 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! Looks good to me, just left some minor comments. Also, since this a behavior change we need to add a note to Misc/NEWS.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/netrc.rst Outdated

@berkerpeksag berkerpeksag Mar 15, 2017

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.

I think the "if :envvar:HOME is not set [...]" part should be moved to the main documentation block and the versionchanged annotation should only mention that it now uses expanduser() to determine the location of the .netrc file if *file* is not passed.

Sorry, something went wrong.

Uh oh!

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

@dmerejkowsky dmerejkowsky Mar 15, 2017

Copy link
Copy Markdown
Author

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.

Makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/netrc.rst Outdated

@berkerpeksag berkerpeksag Mar 15, 2017

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.

Double spaces before "will".

Sorry, something went wrong.

Uh oh!

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

@dmerejkowsky dmerejkowsky Mar 15, 2017

Copy link
Copy Markdown
Author

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.

oups, sorry :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_netrc.py Outdated

@berkerpeksag berkerpeksag Mar 15, 2017

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.

No need to use unlikely_file here. Just pass "unlikely_netrc" to the constructor.

Sorry, something went wrong.

Uh oh!

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

@dmerejkowsky dmerejkowsky Mar 15, 2017

Copy link
Copy Markdown
Author

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.

agreed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch from 7119217 to 62ff357 Compare March 15, 2017 20:05
@dmerejkowsky

dmerejkowsky commented Mar 24, 2017

Copy link
Copy Markdown
Author

@berkerpeksag rebased and conformed with your latest comments
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch from 62ff357 to 86a9333 Compare March 24, 2017 13:19
zhangyangyu
zhangyangyu reviewed Apr 10, 2017
View reviewed changes
Comment thread Doc/library/netrc.rst Outdated

ghost Apr 10, 2017

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.

Two spaces before "If the file ...".

Sorry, something went wrong.

Uh oh!

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

ghost Apr 10, 2017

Copy link
Copy Markdown
Author

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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
zhangyangyu
zhangyangyu reviewed Apr 10, 2017
View reviewed changes
Comment thread Misc/NEWS Outdated

ghost Apr 10, 2017

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.

Uppercase please and colon at end. And instead of fixing, I'd rather something like "Use os.path.expanduser rather than barely checking $HOME is set or not when no argument is given to netrc.netrc().". Ignore my wording, I am not a native speaker. :-(

Sorry, something went wrong.

Uh oh!

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

ghost Apr 10, 2017

Copy link
Copy Markdown
Author

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.

Also done. It's a bit long, but I hope it documents precisely the new behavior

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch 2 times, most recently from df1f1bc to 09abcaf Compare April 10, 2017 11:28
@dmerejkowsky

ghost commented Apr 10, 2017 •
edited by dmerejkowsky
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

@zhangyangyu Thanks for the suggestions! It's worth making sure the documentation is as best as possible :)
Rebased and changed as requested.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zhangyangyu
zhangyangyu reviewed Apr 10, 2017
View reviewed changes
Comment thread Misc/NEWS Outdated

ghost Apr 10, 2017

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.

Too wordy. Just

bpo-28334: Use os.path.expanduser() to find the ~/.netrc file in netrc.netrc().
If the file does not exist, a FileNotFoundError is raised.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka
serhiy-storchaka requested changes Apr 10, 2017
View reviewed changes
Comment thread Doc/library/netrc.rst Outdated

ghost Apr 10, 2017

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.

There are many causes why the file can't be read. It can be a directory or broken symlink, or user can not have permissions to read it. Different OSError subclasses are raised in these cases. I think no need to specify this is explicitly. Exceptions that can be raised by the function are rarely documented in Python docs.

Sorry, something went wrong.

Uh oh!

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

ghost Apr 11, 2017

Copy link
Copy Markdown
Author

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.

Fair enough. I removed the sentence.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_netrc.py Outdated

ghost Apr 10, 2017

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 doesn't test a new behavior. Needed a test for the case when HOME is not set.

Sorry, something went wrong.

Uh oh!

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

ghost Apr 11, 2017

Copy link
Copy Markdown
Author

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 doesn't test a new behavior

I think it does. It checks the type of the exception raised, which is the whole point of this patch,
getting rid of the weird 'OSError' about HOME not set ...

Needed a test for the case when HOME is not set.

Yeah I was not sure about this one. I've added a test where I monkeypatch os.path.expanduser, but I'm not sure it's useful. My guts tell me such a test is too close to the implementation.

Sorry, something went wrong.

Uh oh!

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

ghost Apr 26, 2017 •
edited by berkerpeksag
Loading

Uh oh!

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

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.

Serhiy is right that we need a test for this case and unless I'm missing something you can do it without doing any mocking:

with support.EnvironmentVarGuard() as environ:
    environ.unset('HOME')
    self.assertRaises(FileNotFoundError, netrc.netrc)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dmerejkowsky
dmerejkowsky force-pushed the netrc-home branch from 09abcaf to 1d544f5 Compare April 11, 2017 08:14
@dmerejkowsky

ghost commented Apr 11, 2017

Copy link
Copy Markdown
Author

Wrote a test monkeypatching os.path.expanduser. Maybe we can keep it after all.
You tell me ;)
All reactions

Sorry, something went wrong.

Uh oh!

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

ghost commented Apr 26, 2017

Copy link
Copy Markdown
Author

@serhiy-storchaka @berkerpeksag @terryjreedy gentle reminder :)
All reactions

Sorry, something went wrong.

Uh oh!

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

ghost commented Apr 26, 2017

Copy link
Copy Markdown
Contributor

@dmerejkowsky For reminding, you don't need to squash your commit every change makes, all commit will be squash when the PR is merged. See devguide last paragraph.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
berkerpeksag
berkerpeksag reviewed Apr 26, 2017
View reviewed changes
Comment thread Lib/test/test_netrc.py Outdated

ghost Apr 26, 2017

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.

Mocking os.path.expanduser is not needed here in my opinion. If you can change the test to

def test_home_not_set(self):
    with support.EnvironmentVarGuard() as environ:
        environ.unset('HOME')
        self.assertRaises(FileNotFoundError, netrc.netrc)

then we are good to go! :)

Sorry, something went wrong.

Uh oh!

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

ghost Apr 27, 2017

Copy link
Copy Markdown
Author

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.

I'm afraid not. If we unset HOME and ~/.netrc does exist on the developer box, os.path.expanduser will read /etc/passwd using pwd (see [1]) and no exception will be raised. I'd prefer the tests to not depend on what's on the home directory of the user running the tests.

[1]: https://github.com/python/cpython/blob/master/Lib/posixpath.py#L245-L247

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
berkerpeksag
berkerpeksag reviewed Apr 26, 2017
View reviewed changes
Comment thread Misc/NEWS Outdated

ghost Apr 26, 2017

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.

~/.netrc -> ``~/.netrc``

(otherwise our doc build will complain)

Sorry, something went wrong.

Uh oh!

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

ghost Apr 27, 2017

Copy link
Copy Markdown
Author

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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@berkerpeksag

ghost commented Apr 26, 2017

Copy link
Copy Markdown
Member

@dmerejkowsky and just one more comment: Please add your name to Misc/ACKS.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka requested changes Apr 27, 2017
View reviewed changes
Comment thread Doc/library/netrc.rst Outdated

ghost Apr 27, 2017

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.

Maybe "as determined"? expanduser() itself doesn't return the user's home directory.

Or "as returned by os.path.expanduser('~') ", but this exposes more implementation details and is harder to format as a reference.

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017

Copy link
Copy Markdown
Author

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.

I like 'determined', thanks :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/netrc.py Outdated

ghost Apr 27, 2017

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.

'/' is Posix specific pathname separator. Windows accepts it too, but it is better to use the native pathname separator, os.sep. Or use os.path.join():

os.path.expanduser(os.path.join("~", ".netrc"))

or

os.path.join(os.path.expanduser("~"), ".netrc")

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017 •
edited by dmerejkowsky
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

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.

ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/netrc.rst Outdated

ghost Apr 27, 2017

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.

~/.netrc is Posix-specific syntax. Windows users of netrc can even not know what ~/ means. Maybe "of the file :file:'.netrc'" of "of the default netrc file"?

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017

Copy link
Copy Markdown
Author

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.

agreed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_netrc.py Outdated

ghost Apr 27, 2017

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.

Check the content of nrc for testing that the configuration is read from the right file. Add a random data in the fake .netrc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS Outdated

ghost Apr 27, 2017

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.

Too long line.

Add "Patch by Dimitri Merejkowsky."

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017

Copy link
Copy Markdown
Author

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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS Outdated

ghost Apr 27, 2017

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.

Is this a new behavior?

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017

Copy link
Copy Markdown
Author

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.

Nope, fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_netrc.py Outdated

ghost Apr 27, 2017

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 is too fragile. If rather than os.path.expanduser("~/.netrc") use one of variants suggested by me, the test will be broken.

I would monkey-patch expanduser with a code like the following:

orig_expanduser = os.path.expanduser
called = []
def fake_expanduser(s):
    called.append(s)
    environ.set('HOME', d)
    result = orig_expanduser(s)
    environ.unset('HOME')
    return result
with test.support.swap_attr(os.path, 'expanduser', fake_expanduser):
    nrc = netrc.netrc()
self.assertTrue(called)

Sorry, something went wrong.

Uh oh!

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

ghost Apr 28, 2017

Copy link
Copy Markdown
Author

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.

You're right, test is less likely to break this way.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
berkerpeksag
berkerpeksag approved these changes Nov 24, 2017
View reviewed changes
@bedevere-bot bedevere-bot added the awaiting merge label Nov 24, 2017
@berkerpeksag

ghost commented Nov 24, 2017

Copy link
Copy Markdown
Member

I've fixed merge conflicts, created NEWS entry with blurb and made some cosmetic changes. Is there anything left to do here?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 24, 2017
View reviewed changes
Comment thread Lib/test/test_netrc.py Outdated

ghost Nov 24, 2017

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.

Sorry, I don't understand how this test is related to the case when HOME is not set. Why not use the simple test suggested by @berkerpeksag above?

Sorry, something went wrong.

Uh oh!

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

ghost Nov 24, 2017 •
edited by berkerpeksag
Loading

Uh oh!

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

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.

My suggested test won't work, because of the following code in expanduser:

if 'HOME' not in os.environ:
    import pwd
    userhome = pwd.getpwuid(os.getuid()).pw_dir

I have a .netrc file in my HOME directory so the test won't pass:

def test_foo(self):
    with support.EnvironmentVarGuard() as environ:
        environ.unset('HOME')
        self.assertRaises(FileNotFoundError, netrc.netrc)
======================================================================
FAIL: test_foo (test.test_netrc.NetrcTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/berker/projects/cpython/master/Lib/test/test_netrc.py", line 34, in test_foo
    self.assertRaises(FileNotFoundError, netrc.netrc)
AssertionError: FileNotFoundError not raised by netrc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

ghost Nov 24, 2017

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, now I see that this is the simplest platform-independent test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dimitri Merejkowsky
bpo-28334: fix netrc not working when $HOME is not set
e8dde32 
Also add more tests to check that proper exceptions are raised
@berkerpeksag
berkerpeksag force-pushed the netrc-home branch from 4605b07 to e8dde32 Compare November 24, 2017 08:50
@berkerpeksag

ghost commented Nov 24, 2017

Copy link
Copy Markdown
Member

For some reason, Travis CI didn't pick up latest changes:

/home/travis/build/python/cpython/Doc/library/pyexpat.rst:872:Footnote [#] is not referenced.

Serhiy fixed it a while ago and I don't get any warnings in my development environment.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 24, 2017
View reviewed changes
@berkerpeksag

ghost commented Nov 24, 2017

Copy link
Copy Markdown
Member

Ok, this looks like a Travis bug to me. I've opened PR #4537 and documentation build passed there.

@dmerejkowsky could you please create a new PR? If I merge PR #4537, GitHub overwrite author information and set me as author.
All reactions

Sorry, something went wrong.

Uh oh!

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

ghost commented Nov 24, 2017

Copy link
Copy Markdown
Author

@dmerejkowsky could you please create a new PR? If I merge PR #4537, GitHub overwrite author information and set me as author.

It's not a problem for me :) It was very nice of you to finish up my pull request, so by all means just merge your own and let GitHub set you as author of the patch, you deserve it.

What matters to me is to be listed in the ACKS file 😛
All reactions

Sorry, something went wrong.

Uh oh!

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

ghost commented Nov 25, 2017

Copy link
Copy Markdown
Member

I've just merged PR #4537. Thank you for your work again, @dmerejkowsky. It took more than a year but we eventually fixed the bug :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag closed this Nov 25, 2017
@gvanrossum gvanrossum mentioned this pull request Aug 22, 2023
heap-use-after-free in _PyFunction_LookupByVersion #108253
Closed
This was referenced Feb 11, 2025
segmentfault when pip installing setuptools #129992
Closed
segmentfault when pip installing setuptools #129993
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@zhangyangyu zhangyangyu zhangyangyu approved these changes
@terryjreedy terryjreedy Awaiting requested review from terryjreedy

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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.

8 participants

@dmerejkowsky @the-knights-who-say-ni @zhangyangyu @louisom @berkerpeksag @serhiy-storchaka @Mariatta @bedevere-bot
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.