Code review comment for lp://qastaging/~raxnetworking/nova/melange

Revision history for this message
Rajaram Mallya (rajarammallya) wrote :

> Limiting coupling points like this is something that I do myself and I didn't
> quite catch on that you were doing it for that reason. I'm fine with that
> strategy in general, although I don't know you gain a lot by doing it here.
>
> I would suggest going this route to comply with HACKING:
>
> from openstack.common import config
>
>
> parse_options = config.parse_options
> add_log_options = config.add_log_options
> add_common_options = config.add_common_options
> load_paste_config = config.load_paste_config
> setup_logging = config.setup_logging
> load_paste_app = config.load_paste_app
> get_option = config.get_option
>
>
> Although at this point you're not pylint variable name compliant because
> module-level labels need to be UpperCamelCase I believe.

Nice approach, and more than HACKING compliance, I like the way this makes things more explicit.
With this, there wont be any misunderstandings about these being unused imports.
We'll use this approach, even though it would cause pylint voilations.

> > HTTPNotAcceptable because the version might be coming from the ACCEPT
> headers
> > as well.
> > (The openstack Api1.1 spec mentioned that versions can come from a header,
> > hence
> > we also parse accept headers looking for a version)
>
> This is a fine error if it is talking about headers but I don't think the
> method knows if it's parsed the API version from headers or from the URL
> right? If the API version is given in the Accept header and it's wrong then
> this is valid, if the API version is parsed from the URL then this isn't a
> valid exception. Not sure if that case is covered but I did forget that the
> version might be specified in the Accept header so there are absolutely cases
> where this works.

The HTTPNotAcceptable is raised only when accept headers contain the versions.
If the version is specified in the url, then it doesn't get into this code path at all.
Sorry if my comments might have led you to misunderstand this.

« Back to merge proposal