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

Add optional context_object_name to TemplateColumn and make extra_context optionally callable #931

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

Conversation

jieter
Copy link
Owner

@jieter jieter commented Oct 17, 2023

fixes: #928

@jieter jieter force-pushed the feature/improve-template-column branch from 475dd3d to fe78309 Compare October 17, 2023 12:12
@danielvdp danielvdp self-requested a review October 25, 2023 13:24
Copy link
Collaborator

@danielvdp danielvdp left a comment

Choose a reason for hiding this comment

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

Interessant. Ik heb wat vragen en suggesties

@@ -14,7 +15,9 @@ class TemplateColumn(Column):
Arguments:
template_code (str): template code to render
template_name (str): name of the template to render
extra_context (dict): optional extra template context
context_object_name (str): name of the context variable to pas the record in, defaults to "record".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
context_object_name (str): name of the context variable to pas the record in, defaults to "record".
context_object_name (str): name of the context variable to pass the record in, defaults to "record".

Copy link
Collaborator

Choose a reason for hiding this comment

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

"to pass the record in" klinkt gek. Wellicht "name of the context variable that represents the record, defaults to "record". ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Suggested change
context_object_name (str): name of the context variable to pas the record in, defaults to "record".
context_object_name (str): name of the context variable that represents the record, defaults to "record".

Comment on lines +19 to +20
extra_context (dict): optional extra template context. If a callable is passed, it will be called with
optional record, table, value, bound_column arguments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
extra_context (dict): optional extra template context. If a callable is passed, it will be called with
optional record, table, value, bound_column arguments.
extra_context (dict): optional extra template context. Any callables passed will be called with the following
optional arguments: record, table, value, and bound_column.

Copy link
Collaborator

Choose a reason for hiding this comment

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

kan je meer dan 1 callable meegeven in de dict?

Copy link
Collaborator

Choose a reason for hiding this comment

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

gegeven mijn verwarring over extra_context hieronder kunnen we niet zeggen:
extra_context: accepts dict or callable. If a called is passed it will be called with the following optional arguments: record, ...

"value": value,
"row_counter": kwargs["bound_row"].row_counter,
}
additional_context.update(self.extra_context)

extra_context = self.extra_context
Copy link
Collaborator

Choose a reason for hiding this comment

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

ik vind "additional" en "extra" content wel dubbelop? Waar is additional voor en waar is extra voor? Kunnen we de namen niet wat aanpassen in termen van waar ze voor dienen?

additional_context.update(self.extra_context)

extra_context = self.extra_context
if callable(extra_context):
Copy link
Collaborator

Choose a reason for hiding this comment

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

ik had uit de docstring begrepen dat extra_context een dict was - dus dan zou ik de callable meegeven in een dict ... maar blijkbaar moet je een callable toewijzen aan extra_context? Dus voor mij is de docstring van verwarrend

Copy link
Owner Author

Choose a reason for hiding this comment

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

You should return a callable returning a dict.

table = Table([{"name": "Bob"}])
self.assertEqual(list(table.as_values()), [["Name"], ["Bob"]])

def test_extra_context_callable(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test voor extra context dict was er al toch?

def test_extra_context_callable(self):
class Table(tables.Table):
size = tables.TemplateColumn(
"{{ size }}", extra_context=lambda record: {"size": record["clothes"]["size"]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ik zou het wel beter vinden als er meer optional arguments getest zouden worden? table / value / bound column - ik zie niet goed hoe dat hier terug komt.

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.

Feature request: Make TemplateColumn more dynamic
2 participants