Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fisclimate#1348: Add column attributes #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tjlevel12
Copy link
Contributor

This PR cleans up the Column attributes a bit.

Currently, Column attributes are configured via initialization parameters, which results in Columns being created and configured similarly to the following:

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

This method works, but it's not clear what configuration options are being changed and when. Have we customized the default can_sort value, or is False the default value in Column?

By switching to a system based on class attributes, Column configuration becomes much simpler:

class SomeColumn(Column):
    can_sort = False
    link_label = None

Here we can clearly see that we are customizing the can_sort and link_label attributes.

It should be noted that this is likely a breaking change. Applications using the webgrid library will likely need to be updated to use the new class attributes instead of the initialization parameters. The version number has been bumped a minor revision to help indicate this change.

@codecov
Copy link

codecov bot commented Dec 6, 2017

Codecov Report

Merging #30 into master will decrease coverage by 4.07%.
The diff coverage is 92.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
- Coverage    89.2%   85.13%   -4.08%     
==========================================
  Files          23       17       -6     
  Lines        3576     3498      -78     
  Branches      324      319       -5     
==========================================
- Hits         3190     2978     -212     
- Misses        327      465     +138     
+ Partials       59       55       -4
Impacted Files Coverage Δ
webgrid/utils.py 50% <66.66%> (+16.66%) ⬆️
webgrid/tests/test_utils.py 97.61% <97.61%> (ø)
webgrid/tests/test_blazeweb.py 6.25% <0%> (-87.5%) ⬇️
webgrid/renderers.py 65.26% <0%> (-24.25%) ⬇️
webgrid_blazeweb_ta/views.py
webgrid_blazeweb_ta/tasks/init_db.py
webgrid_blazeweb_ta/model/orm.py
webgrid/blazeweb.py
webgrid_blazeweb_ta/tests/grids.py
webgrid_blazeweb_ta/config/settings.py
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f5c243...4e3b2e1. Read the comment docs.

@guruofgentoo guruofgentoo self-requested a review December 28, 2017 20:44
Copy link
Member

@guruofgentoo guruofgentoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this looks like a good way to clean up some attribute assignment issues, this is indeed a breaking change and would require a fair number of changes across multiple projects.
If we wanted to proceed with this update, we would need to work with some of the existing grids to see if it would be possible to have this improvement without breaking existing code. It seems like it wouldn't be difficult to inspect a column to see which initialization style it has.

@rsyring
Copy link
Member

rsyring commented Dec 28, 2017

Overall, I like where this API change takes us. But the backwards incompatibility is too high a cost with the PR as written.

That being said, it's rather easy to support both methods and, optionally, deprecate the current method.

class Example(object):
    can_sort=True
    def __init__(self, can_sort = _None):
        self.can_sort = self.can_sort if can_sort is _None else can_sort

Obviously, if you want to deprecate the existing functionality, then a bit more work needs to be done, but it could easily be wrapped in a function built for that purpose.

@tjlevel12 I like the initiative you have taken hear to clean things up. We want to keep improving our code base as time permits us to do so. In the future, before you spend time on a large architectural change like this, especially if it breaks backwards compatibility, please talk with Matt or me to make sure we like where you are planning on heading with things. I'd hate to see you put a lot of effort into something like this and then have it rejected for a reason we would have known about ahead of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants