-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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-28806: Improve the netrc library #127
Conversation
8384aca
to
601b3f7
Compare
@zhangyangyu Nice work! I also wrote a tiny patch for |
@dmerejkowsky I wrote this patch months ago, so I have to spend some time picking it up to see if there is any possibility. Happy to see someone also interested in improving netrc. |
OK, then. Hopefully our PRs won't conflict with each other, so this not a big deal. Keep up the good work! |
…tor and traceback objects __setstate__ must accept the state returned by __reduce__. This was not the case for generator and trace-back objects. This commit fixes this. The next commit (merge of issue python#127) adds the relevant test cases. Additionally amends changelog.txt. https://bitbucket.org/stackless-dev/stackless/issues/107
…ling tests. This change is based on f5f98595c6cc63b from 2.7-slp, but it is heavily modified for Python 3. - Pickle 'callable-iterator' objects correctly. Previously the unpickled object had the type '_stackless._wrap.callable-iterator'. - Fix pickling of 'method-wrapper' objects. Previously pickling them caused a SystemError exception. - Fix copy.copy() for 'callable-iterator', 'method', 'dict_keys', 'dict_values' and 'dict_items' objects. Previously the copied object had the type '_stackless._wrap....'. - Fix Stackless pickling tests. The method StacklessTestCase.dumps() didn't pass the pickle protocol to the pickler. - Remove dead code in prickelpit.c. The code was used in older Stackless versions.
…iterator' Disable the Stackless specific code for pickling 'iterator' and 'callable_iterator' objects. C-Python 3.3 already pickles them. Add tests to ensure, that Stackless can still unpickle old pickles. https://bitbucket.org/stackless-dev/stackless/issues/127
elif tt == 'login' or tt == 'user': | ||
login = lexer.get_token() | ||
elif tt == 'account': | ||
account = lexer.get_token() | ||
elif tt == 'password': | ||
if os.name == 'posix' and default_netrc: | ||
if os.name == 'posix' and default_netrc and login != "anonymous": |
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 assumes that login has already been parsed, but the netrc format does not require providing login token before password token. It would be better to perform this check on storing parsed data into self.hosts[entryname], when all machine-related data is parsed and available.
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 please re base
@@ -32,6 +32,12 @@ the Unix :program:`ftp` program and other FTP clients. | |||
|
|||
.. versionchanged:: 3.4 Added the POSIX permission check. | |||
|
|||
.. versionchanged:: 3.7 |
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.
3.8
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 can retarget it for python 3.9
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 not sure, but it may be worth to backport these changes to maintained versions. It fixes several bugs in the .netrc
file parsing.
@@ -32,6 +32,12 @@ the Unix :program:`ftp` program and other FTP clients. | |||
|
|||
.. versionchanged:: 3.4 Added the POSIX permission check. | |||
|
|||
.. versionchanged:: 3.7 |
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.
3.8
continue | ||
if ch == "\"": | ||
for ch in fiter: | ||
if ch != "\"": |
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 revert this condition.
if ch == '"':
return token
And removed continue
.
self.lineno = 1 | ||
self.instream = fp | ||
self.whitespace = "\n\t\r " | ||
self._stack = [] |
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.
It is not a stack, it is a queue. I suggest to rename it to pushback
, as in shlex.lexer
.
for ch in fiter: | ||
if ch in self.whitespace: | ||
continue | ||
if ch == "\"": |
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.
'"'
?
ch = self._read_char() | ||
token += ch | ||
for ch in fiter: | ||
if ch not in self.whitespace: |
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.
The same as above.
lexer.instream.readline() | ||
continue | ||
elif tt == 'machine': | ||
entryname = lexer.get_token() | ||
elif tt == 'default': | ||
entryname = 'default' | ||
elif tt == 'macdef': # Just skip to end of macdefs | ||
elif tt == 'macdef': | ||
entryname = lexer.get_token() |
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 need to read and ignore the rest of the line after the macro name.
raise NetrcParseError( | ||
"Macro definition missing null line terminator.", | ||
file, lexer.lineno) | ||
if line == '\n': |
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.
Add a comment.
@@ -127,10 +172,10 @@ def __repr__(self): | |||
rep = "" | |||
for host in self.hosts.keys(): | |||
attrs = self.hosts[host] | |||
rep = rep + "machine "+ host + "\n\tlogin " + repr(attrs[0]) + "\n" | |||
rep = rep + "machine "+ host + "\n\tlogin " + attrs[0] + "\n" |
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.
Perhaps we need a special function which escapes whitespaces and other special characters?
ch = self._read_char() | ||
token += ch | ||
for ch in fiter: | ||
if ch not in self.whitespace: |
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 suggest to add a flag for denoting whether the terminating whitespace was '\n'
. It will be helpful for comments and macdef
.
@zhangyangyu, would you be able to address Serhiy's review from October? Thanks! |
@csabella I am busy in my job now and this issue has been a long time even myself needs to reacquaint with it. But October seems not that hard to me. Is 3.8 going to release on October? |
@zhangyangyu, sorry, I meant that Serhiy had left a comment last October, not that this needs to be looked at before the next October. The cutoff for 3.8 is this coming Monday (2019-06-03). I hope you still find time to look at it in the next few months. Thanks! |
Hello I can continue this PR if your consider good. Cc @csabella |
Hi @auvipy @csabella @zhangyangyu There is some plan with this PR? What's your opinion? |
@eamanu I think that at this point, you can proceed with fixing the problems uncovered during code review. |
So should this PR be closed? |
I think so. |
Closing this PR since another PR #26330 continues and addresses comments on this thread. All reviews should move to the new PR. |
No description provided.