Code review comment for lp:~jelmer/bzr/safe-open

Revision history for this message
Martin Packman (gz) wrote :

> Hmm, interesting point. I took the name from launchpad, but I guess I've
> just gotten used to it meaning this... What about "bzrlib.hookable_open"
> or "bzrlib.policy_open" ?

Something like that, but unless there's any thought to extending the api beyond just looking at urls, I think that should be reflected in the name. I'm not bursting with ideas though... limited_location_open? restrict_url_open? That this is only branches not repos or trees is also relevant. Perhaps we can bikeshed on IRC?

With a module rename, I'd change SafeBranchOpener to just BranchOpener and fix all the docstrings with 'safe' in them to say something meaningful as well.

> I'm not sure I follow. Do you mean e.g. renaming it to e.g.
> open_with_specific_scheme ?

Yup. Or open_only_scheme, open_branch_with_scheme, something like that.

> This is only really used in the Launchpad testsuite. I guess I could
> just leave it over in the Launchpad code.

Yeah, I thought it probably wasn't in actual use. I don't mind bringing it across anyway, with either an underscore or just a docstring stating that limitation.

« Back to merge proposal