From cb6607895d43a12d9ae1e98f8de3243b9414f0d2 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Tue, 5 Dec 2017 18:25:57 -0500 Subject: [PATCH 1/5] fis-climate#1348 Refactor `Column`s to use class attributes (rather than initialization parameters) as the primary configuration method --- webgrid/__init__.py | 167 +++++++++++++++---------------- webgrid/tests/test_utils.py | 189 ++++++++++++++++++++++++++++++++++++ webgrid/utils.py | 92 ++++++++++++++++++ 3 files changed, 358 insertions(+), 90 deletions(-) create mode 100644 webgrid/tests/test_utils.py diff --git a/webgrid/__init__.py b/webgrid/__init__.py index b081142..f1e1f81 100644 --- a/webgrid/__init__.py +++ b/webgrid/__init__.py @@ -17,6 +17,10 @@ from werkzeug.datastructures import MultiDict from .renderers import HTML, XLS +from .utils import ( + enumerate_class_attributes, + OverridableAttributeProperty +) # conditional imports to support libs without requiring them try: @@ -36,7 +40,7 @@ class _None(object): """ - A sentinal object to indicate no value + A sentinel object to indicate no value """ pass @@ -93,10 +97,16 @@ class Column(object): Column represents the fixed settings for a datagrid column """ _creation_counter = 0 + + # NOTE: `key` and `render_in` have some property magic going on below + key = None + filter = None + can_sort = True xls_width = None - xls_num_format = None xls_style = None - render_in = 'html', 'xls' + xls_num_format = None + render_in = ('html', 'xls') + has_subtotal = False def __new__(cls, *args, **kwargs): col_inst = super(Column, cls).__new__(cls) @@ -109,55 +119,68 @@ def _assign_to_grid(self): grid_cls_cols = grid_locals.setdefault('__cls_cols__', []) grid_cls_cols.append(self) - def __init__(self, label, key=None, filter=None, can_sort=True, - xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, has_subtotal=False, **kwargs): + def __init__(self, label, key=_None, filter=_None, **kwargs): + self.label = label - self.key = key - self.filter = filter - self.filter_for = None - self.filter_op = None - self._create_order = False - self.can_sort = can_sort - self.has_subtotal = has_subtotal + self.expr = None + + for attr in enumerate_class_attributes(self): + if attr in kwargs: + setattr(self, attr, kwargs.pop(attr)) + + if key is not _None: + self.key = key + + if filter is not _None: + self.filter = filter + self.kwargs = kwargs self.grid = None + + # filters can be sent in as a class (not class instance) if needed + if inspect.isclass(self.filter): + if self.expr is None: + raise ValueError('the filter was a class type, but no' + ' column-like object is available from "key" to pass in as' + ' as the first argument') + self.filter = self.filter(self.expr) + + key = OverridableAttributeProperty('key', key) + @key.setter + def key(self, name, value): self.expr = None - if render_in is not _None: - self.render_in = ensure_tuple(render_in) - if xls_width: - self.xls_width = xls_width - if xls_num_format: - self.xls_num_format = xls_num_format - if xls_style: - self.xls_style = xls_style # if the key isn't a base string, assume its a column-like object that # works with a SA Query instance - if key is None: + if value is None: self.can_sort = False - elif not isinstance(key, six.string_types): - self.expr = col = key + + elif isinstance(value, six.string_types): + pass + + else: + self.expr = value + # use column.key, column.name, or None in that order - key = getattr(col, 'key', getattr(col, 'name', None)) + key = getattr(value, 'key', getattr(value, 'name', None)) if key is None: raise ValueError('expected filter to be a SQLAlchemy column-like' ' object, but it did not have a "key" or "name"' ' attribute') - self.key = key - # filters can be sent in as a class (not class instance) if needed - if inspect.isclass(filter): - if self.expr is None: - raise ValueError('the filter was a class type, but no' - ' column-like object is available from "key" to pass in as' - ' as the first argument') - self.filter = filter(self.expr) + value = key + + setattr(self, name, value) + + render_in = OverridableAttributeProperty('render_in', render_in) + @render_in.setter + def render_in(self, name, value): + setattr(self, name, ensure_tuple(value)) def new_instance(self, grid): cls = self.__class__ - column = cls(self.label, self.key, None, self.can_sort, _dont_assign=True) + column = cls(self.label, self.key, None, can_sort=self.can_sort, _dont_assign=True) column.grid = grid column.expr = self.expr @@ -173,13 +196,10 @@ def new_instance(self, grid): else: column.xlwt_stymat = None - # try to be smart about which attributes should get copied to the - # new instance by looking for attributes on the class that have the - # same name as arguments to the classes __init__ method - for argname in inspect.getargspec(self.__init__).args: - if argname != 'self' and hasattr(self, argname) and \ - argname not in ('label', 'key', 'filter', 'can_sort'): - setattr(column, argname, getattr(self, argname)) + # copy the class attributes to the newly-instantiated column + for attr in enumerate_class_attributes(self): + if attr not in ('can_sort', 'filter', 'key', 'label'): + setattr(column, attr, getattr(self, attr)) return column @@ -274,13 +294,7 @@ def xlwt_stymat_calc(self, value): class LinkColumnBase(Column): link_attrs = {} - - def __init__(self, label, key=None, filter=None, can_sort=True, - link_label=None, xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, **kwargs): - Column.__init__(self, label, key, filter, can_sort, xls_width, - xls_style, xls_num_format, render_in, **kwargs) - self.link_label = link_label + link_label = None def render_html(self, record, hah): url = self.create_url(record) @@ -295,16 +309,9 @@ def create_url(self, record): class BoolColumn(Column): - - def __init__(self, label, key_or_filter=None, key=None, can_sort=True, - reverse=False, true_label='True', false_label='False', - xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, **kwargs): - Column.__init__(self, label, key_or_filter, key, can_sort, xls_width, - xls_style, xls_num_format, render_in, **kwargs) - self.reverse = reverse - self.true_label = true_label - self.false_label = false_label + false_label = 'False' + reverse = False + true_label = 'True' def format_data(self, data): if self.reverse: @@ -315,23 +322,12 @@ def format_data(self, data): class YesNoColumn(BoolColumn): - - def __init__(self, label, key_or_filter=None, key=None, can_sort=True, - reverse=False, xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, **kwargs): - BoolColumn.__init__(self, label, key_or_filter, key, can_sort, reverse, - 'Yes', 'No', xls_width, xls_style, xls_num_format, render_in, **kwargs) + false_label = 'No' + true_label = 'Yes' class DateColumnBase(Column): - - def __init__(self, label, key_or_filter=None, key=None, can_sort=True, - html_format=None, xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, **kwargs): - Column.__init__(self, label, key_or_filter, key, can_sort, xls_width, - xls_style, xls_num_format, render_in, **kwargs) - if html_format: - self.html_format = html_format + html_format = None def render_html(self, record, hah): data = self.extract_and_format_data(record) @@ -387,28 +383,19 @@ class TimeColumn(DateColumnBase): class NumericColumn(Column): + curr = '' + dp = '.' + format_as = 'general' + neg = '-' + places = 2 + pos = '' + sep = ',' + trailneg = '' xls_fmt_general = '#,##0{dec_places};{neg_prefix}-#,##0{dec_places}' xls_fmt_accounting = '_($* #,##0{dec_places}_);{neg_prefix}_($* (#,##0{dec_places})' + \ ';_($* "-"??_);_(@_)' xls_fmt_percent = '0{dec_places}%;{neg_prefix}-0{dec_places}%' - - def __init__(self, label, key_or_filter=None, key=None, can_sort=True, - reverse=False, xls_width=None, xls_style=None, xls_num_format=None, - render_in=_None, format_as='general', places=2, curr='', - sep=',', dp='.', pos='', neg='-', trailneg='', - xls_neg_red=True, has_subtotal=False, **kwargs): - Column.__init__(self, label, key_or_filter, key, can_sort, xls_width, - xls_style, xls_num_format, render_in, - has_subtotal, **kwargs) - self.places = places - self.curr = curr - self.sep = sep - self.dp = dp - self.pos = pos - self.neg = neg - self.trailneg = trailneg - self.xls_neg_red = xls_neg_red - self.format_as = format_as + xls_neg_red = True def html_decimal_format_opts(self, data): return ( diff --git a/webgrid/tests/test_utils.py b/webgrid/tests/test_utils.py new file mode 100644 index 0000000..86378fd --- /dev/null +++ b/webgrid/tests/test_utils.py @@ -0,0 +1,189 @@ +from nose.tools import eq_ + +from webgrid.utils import ( + enumerate_class_attributes, + OverridableAttributeProperty +) + +class Test_enumerate_class_attributes: + def test_attribute_enumeration(self): + class A: + a = 123 + b = 456 + + a = A() + + eq_(enumerate_class_attributes(A), ['a', 'b']) + eq_(enumerate_class_attributes(a), ['a', 'b']) + + def test_subclass_attributes(self): + class A: + a = 123 + + class B(A): + b = 456 + + eq_(enumerate_class_attributes(B), ['a', 'b']) + + def test_methods_not_enumerated(self): + class A: + def foo(self): + pass + + assert 'foo' not in enumerate_class_attributes(A) + + def test_private_members_not_enumerated(self): + class A: + foo = 123 + _bar = 456 + + eq_(enumerate_class_attributes(A), ['foo']) + + def test_sorting(self): + class A: + b = 456 + a = 123 + + eq_(enumerate_class_attributes(A), ['a', 'b']) + + def test_properties(self): + class A: + a = 123 + b = 456 + + @property + def c(self): + return 789 + + eq_(enumerate_class_attributes(A), ['a', 'b', 'c']) + + def test_instance_variables_not_enumerated(self): + class A: + a = 123 + + def __init__(self): + self.b = 456 + + a = A() + + eq_(enumerate_class_attributes(a), ['a']) + + +class Test_OverridableAttributeProperty: + def test_class_value(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + + eq_(Foo.bar, 123) + + def test_default_instance_value(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + + foo = Foo() + eq_(foo.bar, 123) + + def test_class_value_writable_by_default(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + + Foo.bar = 345 + eq_(Foo.bar, 345) + + def test_instance_value_writable_by_default(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + + foo = Foo() + foo.bar = 999 + eq_(foo.bar, 999) + + eq_(Foo.bar, 123) + + def test_setter(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + @bar.setter + def bar(self, name, value): + setattr(self, name, 1000 - value) + + foo = Foo() + eq_(foo.bar, 123) + foo.bar = 999 + eq_(foo.bar, 1) + + def test_instance_values_are_per_instance(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + @bar.setter + def bar(self, name, value): + setattr(self, name, 1000 - value) + + a = Foo() + b = Foo() + + a.bar = 10 + eq_(a.bar, 990) + eq_(b.bar, 123) + + def test_behaves_in_init(self): + class Foo: + bar = 123 + + def __init__(self): + self.bar = 200 + + bar = OverridableAttributeProperty('bar', bar) + @bar.setter + def bar(self, name, value): + setattr(self, name, value * 2) + + foo = Foo() + eq_(foo.bar, 400) + + def test_property_inherits_properly(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + @bar.setter + def bar(self, name, value): + setattr(self, name, value * 2) + + class Bar(Foo): + pass + + bar = Bar() + eq_(bar.bar, 123) + bar.bar = 200 + eq_(bar.bar, 400) + + def test_overriden_attribute_is_plain(self): + class Foo: + bar = 123 + + bar = OverridableAttributeProperty('bar', bar) + @bar.setter + def bar(self, name, value): + setattr(self, name, value * 2) + + class Bar(Foo): + bar = 500 + + bar = Bar() + eq_(bar.bar, 500) + bar.bar = 200 + eq_(bar.bar, 200) + diff --git a/webgrid/utils.py b/webgrid/utils.py index a855b4d..facb272 100644 --- a/webgrid/utils.py +++ b/webgrid/utils.py @@ -1,3 +1,26 @@ +import inspect + +def enumerate_class_attributes(cls_or_instance): + """ + Returns a sorted list of the class's attributes + + This method will not return any attribute whose name + begins with an underscore. + + :return: + """ + + if inspect.isclass(cls_or_instance): + cls = cls_or_instance + else: + cls = cls_or_instance.__class__ + + members = inspect.getmembers(cls, lambda attr: not inspect.isroutine(attr)) + return sorted( + attribute + for (attribute, _) in members + if not attribute.startswith('_') + ) def current_url(manager, root_only=False, host_only=False, strip_querystring=False, @@ -49,3 +72,72 @@ def current_url(manager, root_only=False, host_only=False, strip_querystring=Fal retval = retval.replace('https://', 'http://', 1) return retval + + +class OverridableAttributeProperty: + """ + Create an overridable attribute property. + + Say you have a class wherein you'd like to have some data + defined via class attributes. However, you also need to be + able to validate the data that is set on that attribute. + Further, you wish to have the name of the attribute "plain" + (that is, you don't want to have an internal value of `_foo`, + you just want that attribute `foo`). + + This class provides custom setter capabilities while allowing + you to maintain the standard non-internal name. + + This class performs a small bit of magic; of note, you should + be aware that setting a value on one of these properties will + create a instance value (currently named `_{name}_instance_value`) + + Example: + class Foo: + foo = 123 + + . . . + + # create the attribute property + # we need to pass the name (for the 'instance_value'), + # and the current value + foo = OverridableAttributeProperty('foo', foo) + + # create the setter + # note that the setter takes an additional parameter `name`. + # This name should be used when setting the value + @foo.setter + def foo(self, name, value): + # validate `value` + value = . . . + + # save to the internal name + setattr(self, name, value) + + """ + def __init__(self, name, class_value, fset=None): + self.name = name + self.class_value = class_value + self.fset = fset + + @property + def instance_value_name(self): + return '_{}_instance_value'.format(self.name) + + def __get__(self, obj, objtype): + if obj is None: + return self.class_value + + return getattr(obj, self.instance_value_name, self.class_value) + + def __set__(self, obj, value): + if self.fset is None: + setattr(obj, self.instance_value_name, value) + + else: + self.fset(obj, self.instance_value_name, value) + + def setter(self, fset): + return type(self)(self.name, self.class_value, fset) + + From 93bf539dff76b131eb06e4581eab6d719d5d29e2 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Tue, 5 Dec 2017 19:07:37 -0500 Subject: [PATCH 2/5] fis-climate#1348 Bump version number to indicate backwards-incompatibility --- webgrid/version.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webgrid/version.txt b/webgrid/version.txt index a52e041..0ea3a94 100644 --- a/webgrid/version.txt +++ b/webgrid/version.txt @@ -1 +1 @@ -0.1.35 +0.2.0 From 53b3aebeb8fd449002bfb12a93fed5438d2ad7a8 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Tue, 5 Dec 2017 19:13:14 -0500 Subject: [PATCH 3/5] fis-climate#1348 Add a quick description to the readme highlighting the change --- readme.rst | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/readme.rst b/readme.rst index ecb3140..a872626 100644 --- a/readme.rst +++ b/readme.rst @@ -38,3 +38,30 @@ Current Status --------------- Currently beta quality. + +Changes in 0.2.0 +---------------- + +This update includes backwards-incompatible changes that affect the way column classes +are defined. + +From this point forward, column classes should declare their pertinent configuration options +via class attributes, rather than as initialization parameters. + +Pre-0.2.0: + + class SomeColumn(Column): + def __init__(self, label, key=None, filter=None, can_sort=False, + link_label=None, xls_width=None, xls_style=None, xls_num_format=None, + render_in=_None, **kwargs): + Column.__init__(self, label, key, filter, can_sort, xls_width, + xls_style, xls_num_format, render_in, **kwargs) + self.link_label = link_label + + +The equivalent code, beginning in 0.2.0: + + class SomeColumn(Column): + can_sort = False + link_label = None + From 5fa424c32ebeab1d87ce907fa40047850d49fe93 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Wed, 6 Dec 2017 01:14:04 -0500 Subject: [PATCH 4/5] fis-climate#1348 Fix linting errors --- webgrid/__init__.py | 2 ++ webgrid/tests/test_utils.py | 7 ++++++- webgrid/utils.py | 3 +-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/webgrid/__init__.py b/webgrid/__init__.py index f1e1f81..e959e31 100644 --- a/webgrid/__init__.py +++ b/webgrid/__init__.py @@ -146,6 +146,7 @@ def __init__(self, label, key=_None, filter=_None, **kwargs): self.filter = self.filter(self.expr) key = OverridableAttributeProperty('key', key) + @key.setter def key(self, name, value): self.expr = None @@ -174,6 +175,7 @@ def key(self, name, value): setattr(self, name, value) render_in = OverridableAttributeProperty('render_in', render_in) + @render_in.setter def render_in(self, name, value): setattr(self, name, ensure_tuple(value)) diff --git a/webgrid/tests/test_utils.py b/webgrid/tests/test_utils.py index 86378fd..4e61467 100644 --- a/webgrid/tests/test_utils.py +++ b/webgrid/tests/test_utils.py @@ -5,6 +5,7 @@ OverridableAttributeProperty ) + class Test_enumerate_class_attributes: def test_attribute_enumeration(self): class A: @@ -113,6 +114,7 @@ class Foo: bar = 123 bar = OverridableAttributeProperty('bar', bar) + @bar.setter def bar(self, name, value): setattr(self, name, 1000 - value) @@ -127,6 +129,7 @@ class Foo: bar = 123 bar = OverridableAttributeProperty('bar', bar) + @bar.setter def bar(self, name, value): setattr(self, name, 1000 - value) @@ -146,6 +149,7 @@ def __init__(self): self.bar = 200 bar = OverridableAttributeProperty('bar', bar) + @bar.setter def bar(self, name, value): setattr(self, name, value * 2) @@ -158,6 +162,7 @@ class Foo: bar = 123 bar = OverridableAttributeProperty('bar', bar) + @bar.setter def bar(self, name, value): setattr(self, name, value * 2) @@ -175,6 +180,7 @@ class Foo: bar = 123 bar = OverridableAttributeProperty('bar', bar) + @bar.setter def bar(self, name, value): setattr(self, name, value * 2) @@ -186,4 +192,3 @@ class Bar(Foo): eq_(bar.bar, 500) bar.bar = 200 eq_(bar.bar, 200) - diff --git a/webgrid/utils.py b/webgrid/utils.py index facb272..c437ea6 100644 --- a/webgrid/utils.py +++ b/webgrid/utils.py @@ -1,5 +1,6 @@ import inspect + def enumerate_class_attributes(cls_or_instance): """ Returns a sorted list of the class's attributes @@ -139,5 +140,3 @@ def __set__(self, obj, value): def setter(self, fset): return type(self)(self.name, self.class_value, fset) - - From 4e3b2e1746ad45e0e1921fdb3dce9b731e1efff4 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Wed, 6 Dec 2017 02:25:00 -0500 Subject: [PATCH 5/5] fis-climate#1348 Fix Python2 support via new-style classes --- webgrid/tests/test_utils.py | 22 +++++++++++----------- webgrid/utils.py | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/webgrid/tests/test_utils.py b/webgrid/tests/test_utils.py index 4e61467..68b02f6 100644 --- a/webgrid/tests/test_utils.py +++ b/webgrid/tests/test_utils.py @@ -6,7 +6,7 @@ ) -class Test_enumerate_class_attributes: +class Test_enumerate_class_attributes(object): def test_attribute_enumeration(self): class A: a = 123 @@ -70,9 +70,9 @@ def __init__(self): eq_(enumerate_class_attributes(a), ['a']) -class Test_OverridableAttributeProperty: +class Test_OverridableAttributeProperty(object): def test_class_value(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -80,7 +80,7 @@ class Foo: eq_(Foo.bar, 123) def test_default_instance_value(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -89,7 +89,7 @@ class Foo: eq_(foo.bar, 123) def test_class_value_writable_by_default(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -98,7 +98,7 @@ class Foo: eq_(Foo.bar, 345) def test_instance_value_writable_by_default(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -110,7 +110,7 @@ class Foo: eq_(Foo.bar, 123) def test_setter(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -125,7 +125,7 @@ def bar(self, name, value): eq_(foo.bar, 1) def test_instance_values_are_per_instance(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -142,7 +142,7 @@ def bar(self, name, value): eq_(b.bar, 123) def test_behaves_in_init(self): - class Foo: + class Foo(object): bar = 123 def __init__(self): @@ -158,7 +158,7 @@ def bar(self, name, value): eq_(foo.bar, 400) def test_property_inherits_properly(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) @@ -176,7 +176,7 @@ class Bar(Foo): eq_(bar.bar, 400) def test_overriden_attribute_is_plain(self): - class Foo: + class Foo(object): bar = 123 bar = OverridableAttributeProperty('bar', bar) diff --git a/webgrid/utils.py b/webgrid/utils.py index c437ea6..7acd92f 100644 --- a/webgrid/utils.py +++ b/webgrid/utils.py @@ -75,7 +75,7 @@ def current_url(manager, root_only=False, host_only=False, strip_querystring=Fal return retval -class OverridableAttributeProperty: +class OverridableAttributeProperty(object): """ Create an overridable attribute property.