Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1756)

Issue 2874041: Fix issue 9995 - distutils forces developers to store password in cleartext

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 2 months ago by techtonik
Modified:
10 years, 11 months ago
Reviewers:
merwok
CC:
report_bugs.python.org
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : Proper upload (previous patchset was from my own Mercurial copy) #

Total comments: 10

Patch Set 3 : Remove docstring comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -6 lines) Patch
M command/register.py View 1 1 chunk +5 lines, -5 lines 0 comments Download
M command/upload.py View 1 1 chunk +3 lines, -1 line 1 comment Download
M dist.py View 1 1 chunk +1 line, -0 lines 0 comments Download
M tests/test_register.py View 1 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 5
merwok
Looks globally good to me. http://codereview.appspot.com/2874041/diff/2001/cmd.py File cmd.py (right): http://codereview.appspot.com/2874041/diff/2001/cmd.py#newcode55 cmd.py:55: :param distutils.dist.Distribution dist: distribution ...
11 years, 2 months ago (2010-11-08 17:17:11 UTC) #1
techtonik
I don't know when where will be time to redo the patch, but it would ...
11 years, 2 months ago (2010-11-08 18:30:02 UTC) #2
merwok
Further replies. There’s still one comment in test_register you haven’t addressed yet. Tarek, I’d like ...
11 years, 2 months ago (2010-11-08 18:47:39 UTC) #3
techtonik
Fixed docstring. http://codereview.appspot.com/2874041/diff/2001/cmd.py File cmd.py (right): http://codereview.appspot.com/2874041/diff/2001/cmd.py#newcode55 cmd.py:55: :param distutils.dist.Distribution dist: distribution to work with ...
11 years, 2 months ago (2010-11-09 08:28:54 UTC) #4
merwok
10 years, 11 months ago (2011-02-09 22:37:14 UTC) #5
>>>> http://codereview.appspot.com/2874041/diff/2001/command/upload.py#newcode53
>>>> command/upload.py:53: if not self.username and self.distribution.username:
>>>> I’d prefer a clearer comparison, please use “is [not] None” and parens.
>>> Are you sure you want an empty username in config file to override name set
from
>>> 'register' command?
>> This is a crazy case.  PyPI won’t allow it to pass, so I’m okay with it.
> That's why I don't want to make distinction between "None" username and empty
> username. They are both the same in this context.

That’s not what I meant.  IMO, None means not provided and an empty string (i.e.
an empty value in the config file) is a bug introduced by the user.


>
http://codereview.appspot.com/2874041/diff/2001/tests/test_register.py#newcod...
> tests/test_register.py:165: inputs = RawInputs('1', 'tarek', 'n')
>> It looks like you’re replying to “username” and “save your login” but not
>> “password”. 
> That's magically patched in setUp method.

I don’t understand.  The RawInput monkey-patch will give '1', 'tarek' and 'n',
which are anwsers to “choice”, “username” and “save password”, but the code
should ask for “choice”, “username”, “password”, “save password”.

http://codereview.appspot.com/2874041/diff/13001/command/upload.py
File command/upload.py (right):

http://codereview.appspot.com/2874041/diff/13001/command/upload.py#newcode53
command/upload.py:53: if not self.username and self.distribution.username:
A bugfix should not change the API like this.  Either make it a private name
like _username, or (better) use dist.get_command_obj('register').username
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b