-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix NameError in idpicker.py on Python 3. #8
Conversation
setup.py
Outdated
setup( | ||
name='zope.pluggableauth', | ||
version='2.1.1.dev0', | ||
version='2.2.1.dev0', |
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.
Should be 2.2.0.dev0
.
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.
Whoops. Thanks.
================================ | ||
================================== | ||
Pluggable-Authentication Utility | ||
================================== | ||
|
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.
What is the rationale for changing the RST headers in this 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.
All the files need to be consistent with each other in order to be combined.
orig = name | ||
while (not name) or (name in self.context): | ||
i += 1 | ||
name = orig+str(i) | ||
name = orig + str(i) |
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.
How is this not going to break where text_type
is not the same as str
?
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's not going to break mostly through luck 😄
On Python 3, text_type
is str
and str
+ str
works out as expected.
>>> u'abc' + str(123)
'abc123'
On Python 2, text_type
is unicode
. unicode
+ str
does unicode-promotion on the str
argument; the str
of an int is only going to have ASCII characters and so will always be promotable to unicode.
>>> u'abc' + str(123)
u'abc123'
FWIW, zope.container.contained.NameChooser
(from which this class derives) also makes the exact same assumptions in its chooseName
method, freely combining unicode
and str
:
# convert to unicode and remove characters that checkName does not allow
if isinstance(name, bytes):
name = name.decode()
if not isinstance(name, unicode):
try:
name = unicode(name)
except:
name = u''
name = name.replace('/', '-').lstrip('+@')
...
# for an existing name, append a number.
# We should keep client's os.path.extsep (not ours), we assume it's '.'
dot = name.rfind('.')
if dot >= 0:
suffix = name[dot:]
name = name[:dot]
else:
suffix = ''
n = name + suffix
i = 1
while n in container:
i += 1
n = name + u'-' + str(i) + suffix
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.
"where text_type
is not str
" == Python 2, yes?
$ python2
>>> u'\N{SNOWMAN}-' + str(1)
u'\u2603-1'
str(number) gives pure-ASCII strings which can be automatically promoted to unicode on Python 2.
Fixes #7. Also a number of small updates: - Run all the doctests. - Enable test coverage reporting. - Add support for Python 3.6. - Modernize tox.ini - Use pip on Travis and enable Travis caching. - Run all doctests with consistent flags and checker - Use renormalizing.IGNORE_EXCEPTION_MODULE_IN_PYTHON2 instead of manually normalizing one exception - Rename .txt to .rst and include them in the published docs. Fix headers to be consistent to enable this. - List PyPy in the classifiers.
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.
LGTM
@jamadden: you now have PyPI rights for zope.pluggableauth |
Thanks for the reviews! |
Fixes #7.
There were tests for this, they just weren't being run, so make sure to run them.
Also a number of small updates:
manually normalizing one exception
headers to be consistent to enable this.