[MERGE] Add get_username() call to the UIFactory.

Vincent Ladeuil v.ladeuil+lp at free.fr
Wed Apr 1 07:37:10 BST 2009


>>>>> "Jelmer" == Jelmer Vernooij <jelmer at samba.org> writes:

    Jelmer> Thanks again for the quick review; I've fixed the issues you raised,
    Jelmer> but I'm not sure about the last one. 

    Jelmer> On Tue, Mar 31, 2009 at 05:14:39PM +0200, Vincent Ladeuil wrote:
    >> >>>>> "Jelmer" == Jelmer Vernooij <jelmer at samba.org> writes:
    Jelmer> +        """
    Jelmer> +        prompt += ': '
    Jelmer> +        prompt = (prompt % kwargs)
    Jelmer> +        self.prompt(prompt)
    Jelmer> +        return self.stdin.readline().rstrip("\n")

    >> get_non_echoed_password can raise NotATerminal and encode the
    >> prompt according to the terminal encoding, I think get_username
    >> should do the same, time to refactor...

    Jelmer> I think that's because get_non_echoed_password() uses getpass.getpass().
    Jelmer> NotATerminal's error message specifically mentions "password"

class NotATerminal(BzrError):

    _fmt = 'Unable to ask for a password without real terminal.'

Yes, it also mentions that it will not be able to ask for
it. What makes you think you will be able to ask for a username
if you can't ask for a password ?

    Jelmer> and get_boolean() works fine without it.

Good catch, it should be fixed too then :)

    Jelmer> Is there any reason why we should raise that
    Jelmer> exception even if it wouldn't cause issues?

Backward compatibility. We may need to rethink how we want to
handle situations where authentication cannot be achieved without
a terminal, but until then, I don't want to break existing cases.

    Jelmer> TestUIFactory provides a custom implementation of
    Jelmer> get_non_echoed_password(); making it provide a mock
    Jelmer> implementation of get_username() that doesn't raise
    Jelmer> NotATerminal seems kind of pointless as we would only
    Jelmer> be testing the mock implementation, not the actual
    Jelmer> one.

That's a different issue.

If you mean NotATerminal is tested nowhere, you've found yet
another hole in our test suite, feel free to provide more tests
but I don't require them at that point (a FIXME: will do :)

    Vincent



More information about the bazaar mailing list