Open Stack

I think that both ways of doing this should be supported.

Decorated private methods make sense if the different microversions
have nicely interchangeable bits of functionality but not if one of
the private methods would have to be a no-op. A method which just
passes is noise. Additionally there has been talk (but no code I'm
aware of yet) about having a check that version ranges of decorated
methods are both continuous and non-overlapping.

Explicit inline checks do lose some visual impact, but there will be
cases where it makes sense to do an extra-small something (add a
single key to a response-body dict or similar, for example) and a
couple-line code change is a much simpler code change in that case.

So, I've commented on all the patches in what I think is the correct
sequence to leave both suggestions in the devref.

  MG

On Mon, Mar 16, 2015 at 11:32 AM, Alex Xu <soulxu at gmail.com> wrote:
>
>
> 2015-03-16 12:26 GMT+08:00 Christopher Yeoh <cbkyeoh at gmail.com>:
>>
>> So ultimately I think this is a style issue rather than a technical one. I
>> think there
>> are situations where one way looks clearer than another the other way
>> does. Sorry I can't get around to putting up a couple of examples,
>> ATM but to be clear there is no difference in the end result (no different
>> side effects etc)
>
>
> Emm....one more point for multiple version toplevel method: we should
> declare the version explicitly. multiple version internal method and manual
> version comparison are just hide version declaration into the code.
>
>>
>>
>>
>>
>> On Mon, Mar 16, 2015 at 12:21 PM, Alex Xu <soulxu at gmail.com> wrote:
>>>
>>>
>>>
>>> 2015-03-16 9:48 GMT+08:00 Alex Xu <soulxu at gmail.com>:
>>>>
>>>>
>>>>
>>>> 2015-03-13 19:10 GMT+08:00 Sean Dague <sean at dague.net>:
>>>>>
>>>>> On 03/13/2015 02:55 AM, Chris Friesen wrote:
>>>>> > On 03/12/2015 12:13 PM, Sean Dague wrote:
>>>>> >> On 03/12/2015 02:03 PM, Chris Friesen wrote:
>>>>> >>> Hi,
>>>>> >>>
>>>>> >>> I'm having an issue with microversions.
>>>>> >>>
>>>>> >>> The api_version() code has a comment saying "This decorator MUST
>>>>> >>> appear
>>>>> >>> first (the outermost decorator) on an API method for it to work
>>>>> >>> correctly"
>>>>> >>>
>>>>> >>> I tried making a microversioned static class method like this:
>>>>> >>>
>>>>> >>>      @wsgi.Controller.api_version("2.4")  # noqa
>>>>> >>>      @staticmethod
>>>>> >>>      def _my_func(req, foo):
>>>>> >>>
>>>>> >>> and pycharm highlighted the api_version decorator and complained
>>>>> >>> that
>>>>> >>> "This decorator will not receive a callable it may expect; the
>>>>> >>> built-in
>>>>> >>> decorator returns a special object."
>>>>> >>>
>>>>> >>> Is this a spurious warning from pycharm?  The pep8 checks don't
>>>>> >>> complain.
>>>>> >>>
>>>>> >>> If I don't make it static, then pycharm suggests that the method
>>>>> >>> could
>>>>> >>> be static.
>>>>> >>
>>>>> >> *API method*
>>>>> >>
>>>>> >> This is not intended for use by methods below the top controller
>>>>> >> level.
>>>>> >> If you want conditionals lower down in your call stack pull the
>>>>> >> request
>>>>> >> version out yourself and use that.
>>>>> >
>>>>> > Both the original spec and doc/source/devref/api_microversions.rst
>>>>> > contain text talking about decorating a private method.  The latter
>>>>> > gives this example:
>>>>> >
>>>>> >     @api_version("2.1", "2.4")
>>>>> >     def _version_specific_func(self, req, arg1):
>>>>> >         pass
>>>>> >
>>>>> >     @api_version(min_version="2.5") #noqa
>>>>> >     def _version_specific_func(self, req, arg1):
>>>>> >         pass
>>>>> >
>>>>> >     def show(self, req, id):
>>>>> >         .... common stuff ....
>>>>> >         self._version_specific_func(req, "foo")
>>>>> >         .... common stuff ....
>>>>> >
>>>>> > It's entirely possible that such a private method might not need to
>>>>> > reference "self", and could therefore be static, so I think it's a
>>>>> > valid
>>>>> > question.
>>>>>
>>>>> That's a doc bug, we should change it.
>>>>
>>>>
>>>>
>>>> Actually it is not a bug. It's controversial point in the spec, but
>>>> finally that was keep in the spec.
>>>>
>>>> http://specs.openstack.org/openstack/nova-specs/specs/kilo/approved/api-microversions.html
>>>>
>>>> The discussion at line 268
>>>> https://review.openstack.org/#/c/127127/7/specs/kilo/approved/api-microversions.rst
>>>
>>>
>>> Submit a patch for devref https://review.openstack.org/164555  Let see
>>> whether we can get agreement....
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>         -Sean
>>>>>
>>>>> --
>>>>> Sean Dague
>>>>> http://dague.net
>>>>>
>>>>>
>>>>> __________________________________________________________________________
>>>>> OpenStack Development Mailing List (not for usage questions)
>>>>> Unsubscribe:
>>>>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>>
>>>>
>>>
>>>
>>>
>>> __________________________________________________________________________
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe:
>>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>