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

[merwok, 2010-11-08 17:17:11]

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 to work with
Please don’t include unrelated changes in your patch.

Also, Python does not use :param: in docstrings.

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

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.

[techtonik, 2010-11-08 18:30:02]

I don't know when where will be time to redo the patch, but it would be nice to
get some answers in meanwhile.

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
On 2010/11/08 17:17:11, merwok wrote:
> Please don’t include unrelated changes in your patch.
> 
> Also, Python does not use :param: in docstrings.

Including this in another patch is too much work to ever happen. What Python
uses?

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

http://codereview.appspot.com/2874041/diff/2001/command/upload.py#newcode53
command/upload.py:53: if not self.username and self.distribution.username:
On 2010/11/08 17:17:11, merwok wrote:
> 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?

[merwok, 2010-11-08 18:47:39]

Further replies.  There’s still one comment in test_register you haven’t
addressed yet.

Tarek, I’d like to have your decision about one line in the upload command. 
(Don’t read the diff in this mail, it’s incomplete, go to Rietveld.)  Thanks in
advance.

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
> Including this in another patch is too much work to ever happen.
We’ll have to revert it before committing then.

> What Python uses?
Something like “*dist* is an instance of blah”.

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

http://codereview.appspot.com/2874041/diff/2001/command/upload.py#newcode53
command/upload.py:53: if not self.username and self.distribution.username:
This is a crazy case.  PyPI won’t allow it to pass, so I’m okay with it.  Tarek,
what do you think?

http://codereview.appspot.com/2874041/diff/2001/tests/test_register.py
File tests/test_register.py (right):

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”.

[techtonik, 2010-11-09 08:28:54]

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
On 2010/11/08 18:47:39, merwok wrote:
> > Including this in another patch is too much work to ever happen.
> We’ll have to revert it before committing then.
> 
> > What Python uses?
> Something like “*dist* is an instance of blah”.

I thought Python uses all Sphinx capabilities. Done.

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

http://codereview.appspot.com/2874041/diff/2001/command/upload.py#newcode53
command/upload.py:53: if not self.username and self.distribution.username:
On 2010/11/08 18:47:39, merwok wrote:
> This is a crazy case.

That's why I don't want to make distinction between "None" username and empty
username. They are both the same in this context.

http://codereview.appspot.com/2874041/diff/2001/tests/test_register.py
File tests/test_register.py (right):

http://codereview.appspot.com/2874041/diff/2001/tests/test_register.py#newcod...
tests/test_register.py:165: inputs = RawInputs('1', 'tarek', 'n')
On 2010/11/08 18:47:39, merwok wrote:
> It looks like you’re replying to “username” and “save your login” but not
> “password”.

That's magically patched in setUp method.

[merwok, 2011-02-09 22:37:14]

>>>> 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