Separate table class definition and instantiation in MultiTableMixin
#876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jieter
reviewed
Dec 26, 2022
django_tables2/views.py
Outdated
Comment on lines
197
to
201
| raise ImproperlyConfigured( | ||
| "You must either specify {0}.tables or override {0}.get_tables_classes()".format( | ||
| type(self).__name__ | ||
| ) | ||
| ) |
There was a problem hiding this comment.
With the currently supported python versions, we can use fstrings:
Suggested change
| raise ImproperlyConfigured( | |
| "You must either specify {0}.tables or override {0}.get_tables_classes()".format( | |
| type(self).__name__ | |
| ) | |
| ) | |
| view = type(self).__name__ | |
| raise ImproperlyConfigured( | |
| f"You must either specify {view}.tables or override {view}.get_tables_classes()" | |
| ) |
django_tables2/views.py
Outdated
|
|
||
| if len(data) != len(self.tables): | ||
| if len(data) != len(tables): | ||
| klass = type(self).__name__ | ||
| raise ImproperlyConfigured("len({}.tables_data) != len({}.tables)".format(klass, klass)) |
There was a problem hiding this comment.
Can you also apply the fstring form to this error message?
tests/test_views.py
Outdated
| class View(tables.MultiTableMixin, TemplateView): | ||
| template_name = "multiple.html" | ||
|
|
||
| def get_tables_classes(self): |
There was a problem hiding this comment.
This test is implicit in checking the get_tables_classes method is called. I think it would be an improvement if we somehow make it explicit, either by returning something we check, or checking if the method is indeed called.
jieter
changed the title
MultiTableMixin
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a
get_tables_classes()method toMultiTableMixinthat is called fromget_tables()in place of always referring to the tables attribute - this makes it easier to define multiple dynamics custom table table classes in the view - like one can forSingleTableMixin.Also added a test to test the new method.
fixes: #875