Code review comment for lp:~jjed/bzr/xdg_basedir_compliance

Revision history for this message
Andrew Bennetts (spiv) wrote :

Thanks for moving this bug along.

I read the linked bug (bug 195397), but didn't see any clear conclusion about what to do with plugins, and this branch doesn't seem to address them. I think they need to be addressed before this lands, because currently they are expected to exist in ~/.bazaar/plugins.

On to the code:

I'm not sure that calling ensure_config_dir_exists() in Config.__init__ is a good idea. Why is that necessary? In general it's nice to defer doing work until it is actually required. And I'm not sure it makes sense for BranchConfig to do this unconditionally.

> + while os.path.exists(user_notice):
> + user_notice = '_'.join((user_notice, 'ALT'))

This seems like overkill for a one-off transition. Is this just being extra careful, or do you expect this to be used? And it seems a shame to be so cautious and then leave a race between the exists check and the file open anyway.

> with open(user_notice, 'w') as file:

This is a Python 2.5-ism (although it needs a __future__ import), but bzr supports Python 2.4+. So update this to use try/finally.

Finally, the docs must be updated before this could be merged, so for the docs and the plugins issue I'm marking this Resubmit.

review: Needs Resubmitting

« Back to merge proposal