[MERGE] Authentication ring implementation (read-only)
Martin Pool
mbp at canonical.com
Sat Nov 3 22:31:43 GMT 2007
Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
I apologize for the delay in merging this. I think the long diff put me
off, though it's not really hard to read.
+ * New ``authentication.conf`` file to define remote servers
credentials.
+ (Vincent Ladeuil)
I'd change that to
... file holding the password or other credentials for remote servers.
This can be used for ssh, sftp, smtp and other supported transports.
- warning('BZREMAIL is deprecated in favor of BZR_EMAIL.
Please update your configuration.')
+ # FIXME: Seems to have been deprecated since more than a
year now,
+ # time to delete ? -- vila 20071019
+ trace.warning('BZREMAIL is deprecated in favor of
BZR_EMAIL.'
+ ' Please update your configuration.')
Please go ahead and give one.
+class AuthenticationConfig(object):
I was surprised this is not inheriting from one of the existing
config classes, but I don't really see a reason for it to do so.
+ super(AuthenticationConfig, self).__init__()
If it doesn't actually *have* a super method, it seems a bit strange or
misleading to call it. I don't know if we have a specific policy on it.
Calling super always to get "always called first" behaviour like Java or
C++ seems not quite right.
+ self._get_filename = authentication_config_filename
It looks like it's assigning a filename, but it's actually assigning a
callable. Adding comments on the member variables would help.
Actually, is there any reason not to just evaluate it when the object's
constructed?
+ self._input = self._get_filename()
+ else:
+ self._get_filename = None
+ self._input = _file
+ a_port = None
+ except ValueError:
+ raise ValueError("'port' not numeric in %s" %
auth_def_name)
If these are coming from the user's configuration file, and represent
configuration errors by the user then
- This should not really be ValueError, because that will give a
traceback. It does not need a custom class.
- Maybe they should be just warnings?
- Maybe they should include the line number, if that can be easily
determined?
- They should give the config file name.
Maybe it would be better to split out the code that matches an entry
against a search query out from the fact it comes from the config for
easier testing.
+ auth = {'host': self._host, 'port': self._port,
+ 'user': user, 'password': password,
+ 'protocol': self._unqualified_scheme,
+ 'path': self._path}
I find these easier to read as
dict(host=self._host, port=self._port, ..)
etc, when all the keys are Python keywords. Less quoting.
=== modified file 'doc/developers/authentication-ring.txt'
You need to selectively move some of this into the user documentation,
now
that it'll be accessible to users. I think this really should be done
before merge, even if it's just a cut of the specification text (which
is
already pretty clear.)
Aside from those (particularly the docs) I would be happy to have it
merged.
For details, see:
http://bundlebuggy.aaronbentley.com/request/%3Cm28x5tyj89.fsf%40free.fr%3E
More information about the bazaar
mailing list