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

Table inheritance do not keep column option #337

Closed
nimch opened this issue Jun 1, 2016 · 10 comments
Closed

Table inheritance do not keep column option #337

nimch opened this issue Jun 1, 2016 · 10 comments

Comments

@nimch
Copy link

nimch commented Jun 1, 2016

Hi there !

First, thank you for sharing your work.

Second, the issue : when inherit from personal Table class, it seems that Column arguments (like verbose_name, orderable, etc) are not kept, right ?

Example:

class MyTable(tables.Table):
    item1 = tables.Column(verbose_name="Nice column name")
    class Meta:
        model = MyModel
        fields = ('item1',)

class MyTableExt(MyTable):
    item2 = tables.Column()
    class Meta(MyTable.Meta):
        fields = ('item1', 'item2')

In MyTableExt, item1 do not have "Nice column name", always right ?

Thanks.

jieter added a commit that referenced this issue Jun 1, 2016
@jieter
Copy link
Owner

jieter commented Jun 1, 2016

Hmm, that seems to be the case indeed. Pushed a failing tests to branch issue-337 based on your example. Will look into it, thanks for reporting!

@jieter
Copy link
Owner

jieter commented Jun 1, 2016

@nimch This is what happens:

The metaclass converts the explicit and implicit columns of MyTable to MyTable.base_columns giving explicit columns higher precedence. When it gets extended into MyTableExt, the metaclass does take into account it's parents base_columns, but the difference in precedence between explicit/implicit columns is lost between those defined in MyTable and MyTableExt. Because there is an implicit definition for item1 in MyTableExt, it overrides the definition in it's parents base_columns.

I'll think about a way to fix this, in the mean time: in this case there is no need to define fields = ('item', 'item2'), if you comment that out it works as expected.

@nimch
Copy link
Author

nimch commented Jun 1, 2016

Unfortunately, your proposal don't solve my issue. I still get "default" verbose_names.
Otherwise, every columns defines with accessor argument is empty, same issue probably.
I have to stop investigation for today but I will continue tomorrow.
See you.

@jieter
Copy link
Owner

jieter commented Jun 1, 2016

@nimch interesting, I did exactly that here, which makes the build pass

@nimch
Copy link
Author

nimch commented Jun 2, 2016

Hello, I'm back !

I'm not convinced of your test code... Where is MyTable definition for example ?

I redo tests with minimalist code to be sure (and not my more complex code hacked).
Here the code of my_table.py :

#!/usr/bin/env python3

from django.db import models
from django_tables2 import Table, Column

class MyModel(models.Model):
    item1 = models.CharField()

class MyTable(Table):
    item1 = Column(verbose_name="nice verbose name")

    class Meta:
        per_page = 20
        model = MyModel
        fields = ('item1',)

class MyTableExt(MyTable):
    item2 = Column(verbose_name="nice verbose name too")

    class Meta(MyTable.Meta):
        per_page = 22

Here Python shell :

>>> from tracadpp import my_table
>>> table = my_table.MyTable(my_table.MyModel.objects.all())
>>> print(table.columns['item1'].verbose_name)
nice verbose name
>>> table_ext = my_table.MyTableExt(my_table.MyModel.objects.all())
>>> print(table_ext.columns['item1'].verbose_name)
item1
>>> print(table_ext.columns['item2'].verbose_name)
nice verbose name too

As you can see, verbose name defined in MyTable is lost in MyTableExt.
It could be important : I'm using Python 3.4, Django 1.8.7 and django-tables2 1.1.0.

@jieter
Copy link
Owner

jieter commented Jun 2, 2016

The diff was collapsed which made the MyTable definition missing see:
https://github.com/bradleyayers/django-tables2/blob/cf63e44bbfae1d08c56578261fd5552c1da33956/tests/columns/test_general.py#L341-L374

The tests run in CI against django 1.8 in python 2.7, 3.3 and 3.4. Your version of django-tables2 shouldn't matter but it would keep things more clear if you upgraded to the latest version.

I just noticed why it fails in your example and works in mine: if you have no class Meta on the child class at all, it works as expected, but as soon as you have an empty class Meta(MyTable.Meta):, the test fails.

jieter added a commit that referenced this issue Jun 2, 2016
@nimch
Copy link
Author

nimch commented Jun 2, 2016

Empty Meta class is not allowed so test fails, this is correct.

Same behaviour here : without any definition of Meta in child class, parent class fields' verbose_name are preserved.

Well, inheritance without the ability of modifying Meta option is less useful, no ? Do you consider this is normal behaviour ?

Thanks for your help.

@jieter
Copy link
Owner

jieter commented Jun 2, 2016

Empty Meta class is not allowed so test fails, this is correct.

It does not fail on it being empty, the test fails on a failed assertion.

I do consider it a bug (issue is still open), but didn't have the time to fix it yet. If you have time and skill to fix it, feel free to open a pull request!

jieter added a commit that referenced this issue Jun 3, 2016
jieter added a commit that referenced this issue Jun 3, 2016
jieter added a commit that referenced this issue Jun 3, 2016
@nimch
Copy link
Author

nimch commented Jun 3, 2016

Hi !
I've just tested your correction and it's ok for me : verbose_name, accessor, etc are preserved in child table.
Good job !

@jieter jieter closed this as completed in 03cb7c5 Jun 3, 2016
@jieter
Copy link
Owner

jieter commented Jun 3, 2016

@nimch thanks for verifying!

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

No branches or pull requests

2 participants