yahoo-eng-team team mailing list archive

[David Lyle <david.lyle@xxxxxx>]

** Changed in: horizon
   Importance: Undecided => Wishlist

** Changed in: horizon
       Status: In Progress => Fix Released

-- 
You received this bug notification because you are a member of Yahoo!
Engineering Team, which is subscribed to OpenStack Dashboard (Horizon).
https://bugs.launchpad.net/bugs/1182931

Title:
  The use of class variables

Status in OpenStack Dashboard (Horizon):
  Fix Released

Bug description:
  I would like to ask is there is any reason for using so much class
  variables in Horizon. Is there? I am quite new to python (ruby was my
  world before) so I don't really know. And there may be something
  inside Horizon that I don't see.

  Using class attributes as default values for instance attributes is
  considered to be an anti-pattern (in any language). As  I checked
  google and it seems this also applies for python community. Actually
  it is anti-pattern to use class variables in most cases, because it is
  not thread save and it can break the interface of objects.

  I am trying to show how it could look like it it were encapsulated in
  object, rather then working with class attributes.

  So this how it looks now
  example 1.
  http://pastebin.com/kxas6AsD

  and it could be rewritten to something like this
  example 2.
  http://pastebin.com/LHFuXWMb

  Now what are the exact advantages of this approach:
  1. 
  Example 2 is much more clear and concise, because now I see by first look exactly what attributes are defined in each level of inheritance tree. 
  So instead of redefining a class variable in my concrete class of some action I rather define the initializer and set the new defaults there. 

  2. 
  It will be consistent way how to set the defaults and the values of the attributes. Now each attribute is set in slightly different manner and I have to always look through a lot of code to see what value is actually getting assigned. This tends to also very buggy.

  3. 
  Relying on the python delegation (it first look to object, then superclass, then superclass...) when I set the defaults can be quite confusing.
  In example 1, although the value of important_attribute is set in FilterAction(), it actually takes the class attribute from  MyGreatFilterAction(FilterAction). Because it takes the class attribute important_attribute, that is closest to the object (so immediate super class) . If we put much more classes to the inheritance tree, this gets more and more confusing.

  4. 
  All is encapsulated to the object, so it doesn't break any pattern and it is thread safe. Now I can for example set the default value that will be different for each user role and it still will be consistent (or something like that),

  
  I really appreciate your work guys. I would be very glad if you could spent some time on this and give me some feedback, and say if you like the idea. And if you will like it then we can make it even nicer. :-)  

  I would gladly make these changes if it will be confirmed. Because I
  know it won't be exactly easy.

  Thank you very much for your help and time.

To manage notifications about this bug go to:
https://bugs.launchpad.net/horizon/+bug/1182931/+subscriptions