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

Moved table instantiation from get_context_data to get_tables #554

Merged
merged 4 commits into from Mar 17, 2018

Conversation

sdolemelipone
Copy link
Contributor

@sdolemelipone sdolemelipone commented Mar 16, 2018

Further to the recent changes to MultiTableMixin by @vCra in issue #538, I would like to suggest the following change: get_tables should create table instances by calling get_tables_data, in an analogous manner to SingleTableMixin, rather than leaving it to get_context_data.

The use case for this is that sometimes it is necessary for table classes to be defined after get_tables_data is called, due to variable columns in the data. In those cases it is preferable to override get_tables to deal with the class instantiation. In the current implementation it is also necessary to override get_context_data in this situation, as that function has a preference on how table classes and their data are instantiated. IMO get_context_data should be concerned with adding the tables to the context, and agnostic about the table construction.

I have attached a PR showing the suggested changes.

@sdolemelipone
Copy link
Contributor Author

Apologies for all the noisy commits, I got tox working now. :-)

@jieter
Copy link
Owner

jieter commented Mar 17, 2018

Makes sense, thanks for the pull. I'll squash the commits to remove the noise ;)

@jieter jieter merged commit 3459dbc into jieter:master Mar 17, 2018
jieter added a commit that referenced this pull request Mar 17, 2018
@sdolemelipone
Copy link
Contributor Author

Thanks! My first accepted PR :-)

@sdolemelipone sdolemelipone deleted the get_tables branch March 23, 2018 10:30
@jieter
Copy link
Owner

jieter commented Mar 23, 2018

👍 well done!

@jieter
Copy link
Owner

jieter commented Mar 26, 2018

released 1.21.2

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.

None yet

2 participants