Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-31 Thread Yair Fried
Hi,
I'd rather not subclass dict directly.
for various reasons adding extra attributes to normal python dict seems prone 
to errors since people will be expecting regular dicts, and on the other hand 
if we want to expand it in the future we might run into problems playing with 
dict methods (such as update)

I suggets (roughly):

class ResponseBody(dict): 
def __init__(self, body=None, resp=None): 
self_data_dict = body or {} 
self.resp = resp 

def __getitem__(self, index):
return self._data_dict[index]


Thus we can keep the previous dict interface, but protect the data and make 
sure the object will behave exactly as we expect it to. if we want it to have 
more dict attributes/method we can add them explicitly


- Original Message -
From: Boris Pavlovic bpavlo...@mirantis.com
To: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.org
Sent: Saturday, August 30, 2014 2:53:37 PM
Subject: Re: [openstack-dev] [qa] Lack of consistency in returning response 
from tempest clients

Sean, 




class ResponseBody(dict): 
def __init__(self, body={}, resp=None): 
self.update(body) 
self.resp = resp 


Are you sure that you would like to have default value {} for method argument 
and not something like: 


class ResponseBody(dict): 
def __init__(self, body=None, resp=None): 
body = body or {} 
self.update(body) 
self.resp = resp 

In your case you have side effect. Take a look at: 
http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument
 

Best regards, 
Boris Pavlovic 


On Sat, Aug 30, 2014 at 10:08 AM, GHANSHYAM MANN  ghanshyamm...@gmail.com  
wrote: 



+1. That will also help full for API coming up with microversion like Nova. 


On Fri, Aug 29, 2014 at 11:56 PM, Sean Dague  s...@dague.net  wrote: 


On 08/29/2014 10:19 AM, David Kranz wrote: 
 While reviewing patches for moving response checking to the clients, I 
 noticed that there are places where client methods do not return any value. 
 This is usually, but not always, a delete method. IMO, every rest client 
 method should return at least the response. Some services return just 
 the response for delete methods and others return (resp, body). Does any 
 one object to cleaning this up by just making all client methods return 
 resp, body? This is mostly a change to the clients. There were only a 
 few places where a non-delete method was returning just a body that was 
 used in test code. 

Yair and I were discussing this yesterday. As the response correctness 
checking is happening deeper in the code (and you are seeing more and 
more people assigning the response object to _ ) my feeling is Tempest 
clients should probably return a body obj that's basically. 

class ResponseBody(dict): 
def __init__(self, body={}, resp=None): 
self.update(body) 
self.resp = resp 

Then all the clients would have single return values, the body would be 
the default thing you were accessing (which is usually what you want), 
and the response object is accessible if needed to examine headers. 

-Sean 

-- 
Sean Dague 
http://dague.net 

___ 
OpenStack-dev mailing list 
OpenStack-dev@lists.openstack.org 
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev 



-- 
Thanks  Regards 
Ghanshyam Mann 


___ 
OpenStack-dev mailing list 
OpenStack-dev@lists.openstack.org 
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev 



___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-30 Thread Andrea Frittoli
+1

keeping the body as a ~dict will help with all existing asserts comparing
dicts in tests.

Andrea
On 30 Aug 2014 06:45, Christopher Yeoh cbky...@gmail.com wrote:

 On Fri, 29 Aug 2014 11:13:39 -0400
 David Kranz dkr...@redhat.com wrote:

  On 08/29/2014 10:56 AM, Sean Dague wrote:
   On 08/29/2014 10:19 AM, David Kranz wrote:
   While reviewing patches for moving response checking to the
   clients, I noticed that there are places where client methods do
   not return any value. This is usually, but not always, a delete
   method. IMO, every rest client method should return at least the
   response. Some services return just the response for delete
   methods and others return (resp, body). Does any one object to
   cleaning this up by just making all client methods return resp,
   body? This is mostly a change to the clients. There were only a
   few places where a non-delete  method was returning just a body
   that was used in test code.
   Yair and I were discussing this yesterday. As the response
   correctness checking is happening deeper in the code (and you are
   seeing more and more people assigning the response object to _ ) my
   feeling is Tempest clients should probably return a body obj that's
   basically.
  
   class ResponseBody(dict):
def __init__(self, body={}, resp=None):
self.update(body)
   self.resp = resp
  
   Then all the clients would have single return values, the body
   would be the default thing you were accessing (which is usually
   what you want), and the response object is accessible if needed to
   examine headers.
  
   -Sean
  
  Heh. I agree with that and it is along a similar line to what I
  proposed here https://review.openstack.org/#/c/106916/ but using a
  dict rather than an attribute dict. I did not propose this since it
  is such a big change. All the test code would have to be changed to
  remove the resp or _ that is now receiving the response. But I think
  we should do this before the client code is moved to tempest-lib.

 +1. this would be a nice cleanup.

 Chris

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-30 Thread GHANSHYAM MANN
+1. That will also help full for API coming up with microversion like Nova.

On Fri, Aug 29, 2014 at 11:56 PM, Sean Dague s...@dague.net wrote:

 On 08/29/2014 10:19 AM, David Kranz wrote:
  While reviewing patches for moving response checking to the clients, I
  noticed that there are places where client methods do not return any
 value.
  This is usually, but not always, a delete method. IMO, every rest client
  method should return at least the response. Some services return just
  the response for delete methods and others return (resp, body). Does any
  one object to cleaning this up by just making all client methods return
  resp, body? This is mostly a change to the clients. There were only a
  few places where a non-delete  method was returning just a body that was
  used in test code.

 Yair and I were discussing this yesterday. As the response correctness
 checking is happening deeper in the code (and you are seeing more and
 more people assigning the response object to _ ) my feeling is Tempest
 clients should probably return a body obj that's basically.

 class ResponseBody(dict):
 def __init__(self, body={}, resp=None):
 self.update(body)
 self.resp = resp

 Then all the clients would have single return values, the body would be
 the default thing you were accessing (which is usually what you want),
 and the response object is accessible if needed to examine headers.

 -Sean

 --
 Sean Dague
 http://dague.net

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




-- 
Thanks  Regards
Ghanshyam Mann
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-30 Thread Boris Pavlovic
Sean,


class ResponseBody(dict):
 def __init__(self, body={}, resp=None):
 self.update(body)
 self.resp = resp



Are you sure that you would like to have default value {} for method
argument and not something like:


class ResponseBody(dict):
def __init__(self, body=None, resp=None):
body = body or {}
self.update(body)
self.resp = resp


In your case you have side effect. Take a look at:
http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument

Best regards,
Boris Pavlovic


On Sat, Aug 30, 2014 at 10:08 AM, GHANSHYAM MANN ghanshyamm...@gmail.com
wrote:

 +1. That will also help full for API coming up with microversion like Nova.


 On Fri, Aug 29, 2014 at 11:56 PM, Sean Dague s...@dague.net wrote:

 On 08/29/2014 10:19 AM, David Kranz wrote:
  While reviewing patches for moving response checking to the clients, I
  noticed that there are places where client methods do not return any
 value.
  This is usually, but not always, a delete method. IMO, every rest client
  method should return at least the response. Some services return just
  the response for delete methods and others return (resp, body). Does any
  one object to cleaning this up by just making all client methods return
  resp, body? This is mostly a change to the clients. There were only a
  few places where a non-delete  method was returning just a body that was
  used in test code.

 Yair and I were discussing this yesterday. As the response correctness
 checking is happening deeper in the code (and you are seeing more and
 more people assigning the response object to _ ) my feeling is Tempest
 clients should probably return a body obj that's basically.

 class ResponseBody(dict):
 def __init__(self, body={}, resp=None):
 self.update(body)
 self.resp = resp

 Then all the clients would have single return values, the body would be
 the default thing you were accessing (which is usually what you want),
 and the response object is accessible if needed to examine headers.

 -Sean

 --
 Sean Dague
 http://dague.net

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




 --
 Thanks  Regards
 Ghanshyam Mann


 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-29 Thread Jay Pipes

On 08/29/2014 10:19 AM, David Kranz wrote:

While reviewing patches for moving response checking to the clients, I
noticed that there are places where client methods do not return any value.
This is usually, but not always, a delete method. IMO, every rest client
method should return at least the response. Some services return just
the response for delete methods and others return (resp, body). Does any
one object to cleaning this up by just making all client methods return
resp, body? This is mostly a change to the clients. There were only a
few places where a non-delete  method was returning just a body that was
used in test code.


Sounds good to me. :)

-jay


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-29 Thread Sean Dague
On 08/29/2014 10:19 AM, David Kranz wrote:
 While reviewing patches for moving response checking to the clients, I
 noticed that there are places where client methods do not return any value.
 This is usually, but not always, a delete method. IMO, every rest client
 method should return at least the response. Some services return just
 the response for delete methods and others return (resp, body). Does any
 one object to cleaning this up by just making all client methods return
 resp, body? This is mostly a change to the clients. There were only a
 few places where a non-delete  method was returning just a body that was
 used in test code.

Yair and I were discussing this yesterday. As the response correctness
checking is happening deeper in the code (and you are seeing more and
more people assigning the response object to _ ) my feeling is Tempest
clients should probably return a body obj that's basically.

class ResponseBody(dict):
def __init__(self, body={}, resp=None):
self.update(body)
self.resp = resp

Then all the clients would have single return values, the body would be
the default thing you were accessing (which is usually what you want),
and the response object is accessible if needed to examine headers.

-Sean

-- 
Sean Dague
http://dague.net

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-29 Thread David Kranz

On 08/29/2014 10:56 AM, Sean Dague wrote:

On 08/29/2014 10:19 AM, David Kranz wrote:

While reviewing patches for moving response checking to the clients, I
noticed that there are places where client methods do not return any value.
This is usually, but not always, a delete method. IMO, every rest client
method should return at least the response. Some services return just
the response for delete methods and others return (resp, body). Does any
one object to cleaning this up by just making all client methods return
resp, body? This is mostly a change to the clients. There were only a
few places where a non-delete  method was returning just a body that was
used in test code.

Yair and I were discussing this yesterday. As the response correctness
checking is happening deeper in the code (and you are seeing more and
more people assigning the response object to _ ) my feeling is Tempest
clients should probably return a body obj that's basically.

class ResponseBody(dict):
 def __init__(self, body={}, resp=None):
 self.update(body)
self.resp = resp

Then all the clients would have single return values, the body would be
the default thing you were accessing (which is usually what you want),
and the response object is accessible if needed to examine headers.

-Sean

Heh. I agree with that and it is along a similar line to what I proposed 
here https://review.openstack.org/#/c/106916/ but using a dict rather 
than an attribute dict. I did not propose this since it is such a big 
change. All the test code would have to be changed to remove the resp or 
_ that is now receiving the response. But I think we should do this 
before the client code is moved to tempest-lib.


 -David

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-29 Thread Miguel Lavalle
Yeah,  Sean's proposal looks great to me


On Fri, Aug 29, 2014 at 10:13 AM, David Kranz dkr...@redhat.com wrote:

 On 08/29/2014 10:56 AM, Sean Dague wrote:

 On 08/29/2014 10:19 AM, David Kranz wrote:

 While reviewing patches for moving response checking to the clients, I
 noticed that there are places where client methods do not return any
 value.
 This is usually, but not always, a delete method. IMO, every rest client
 method should return at least the response. Some services return just
 the response for delete methods and others return (resp, body). Does any
 one object to cleaning this up by just making all client methods return
 resp, body? This is mostly a change to the clients. There were only a
 few places where a non-delete  method was returning just a body that was
 used in test code.

 Yair and I were discussing this yesterday. As the response correctness
 checking is happening deeper in the code (and you are seeing more and
 more people assigning the response object to _ ) my feeling is Tempest
 clients should probably return a body obj that's basically.

 class ResponseBody(dict):
  def __init__(self, body={}, resp=None):
  self.update(body)
 self.resp = resp

 Then all the clients would have single return values, the body would be
 the default thing you were accessing (which is usually what you want),
 and the response object is accessible if needed to examine headers.

 -Sean

  Heh. I agree with that and it is along a similar line to what I proposed
 here https://review.openstack.org/#/c/106916/ but using a dict rather
 than an attribute dict. I did not propose this since it is such a big
 change. All the test code would have to be changed to remove the resp or _
 that is now receiving the response. But I think we should do this before
 the client code is moved to tempest-lib.

  -David


 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [qa] Lack of consistency in returning response from tempest clients

2014-08-29 Thread Christopher Yeoh
On Fri, 29 Aug 2014 11:13:39 -0400
David Kranz dkr...@redhat.com wrote:

 On 08/29/2014 10:56 AM, Sean Dague wrote:
  On 08/29/2014 10:19 AM, David Kranz wrote:
  While reviewing patches for moving response checking to the
  clients, I noticed that there are places where client methods do
  not return any value. This is usually, but not always, a delete
  method. IMO, every rest client method should return at least the
  response. Some services return just the response for delete
  methods and others return (resp, body). Does any one object to
  cleaning this up by just making all client methods return resp,
  body? This is mostly a change to the clients. There were only a
  few places where a non-delete  method was returning just a body
  that was used in test code.
  Yair and I were discussing this yesterday. As the response
  correctness checking is happening deeper in the code (and you are
  seeing more and more people assigning the response object to _ ) my
  feeling is Tempest clients should probably return a body obj that's
  basically.
 
  class ResponseBody(dict):
   def __init__(self, body={}, resp=None):
   self.update(body)
  self.resp = resp
 
  Then all the clients would have single return values, the body
  would be the default thing you were accessing (which is usually
  what you want), and the response object is accessible if needed to
  examine headers.
 
  -Sean
 
 Heh. I agree with that and it is along a similar line to what I
 proposed here https://review.openstack.org/#/c/106916/ but using a
 dict rather than an attribute dict. I did not propose this since it
 is such a big change. All the test code would have to be changed to
 remove the resp or _ that is now receiving the response. But I think
 we should do this before the client code is moved to tempest-lib.

+1. this would be a nice cleanup.

Chris

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev