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

Linkify #590

Merged
merged 8 commits into from
Jul 17, 2018
Merged

Linkify #590

merged 8 commits into from
Jul 17, 2018

Conversation

jieter
Copy link
Owner

@jieter jieter commented Jul 8, 2018

New API to transform any column into a link column:

class MyTable(tables.Table):
    id = tables.Column(linkify=True)
    country = tables.Column(linkify=True)
    start_date = tables.DateTimeColumn(linkify=True)

This allows using specialized columns (like DateColumn) with links, without falling back to LinkColumns, which do not know about dates.

You can also pass a dict, which will be passed thru to django.urls.reverse:

class MyTable(tables.Table):
    id = tables.Column(linkify=dict(viewname='person_detail', args=[A('pk')]))

Still TODO:

  • decide if this is a good idea
  • Add documentation
  • and tests
  • Check for regressions and fix

@dyve
Copy link
Contributor

dyve commented Jul 9, 2018

I don't like chaining. It confuses me (emphasis on I and me, this may be personal preference). More importantly, it tends to have magic rules. Most often "your chained function should always return type X unless [something special], then it's OK to do something else".

Wouldn't it be more pythonic to add an optional parameter url to any Column? If url is not None (and of course should be kwargs, then do exactly the same as you are now proposing to do with the linkify chain.

@jieter jieter force-pushed the linkify branch 2 times, most recently from 4a91de2 to 318960f Compare July 11, 2018 18:35
@jieter jieter force-pushed the linkify branch 2 times, most recently from 3ca5074 to 7802640 Compare July 16, 2018 08:19
@jieter jieter force-pushed the linkify branch 3 times, most recently from ab39bbc to ca7763f Compare July 16, 2018 14:29
``a`` tag. The different ways to define the ``href`` attribute:

- If `True`, the ``record.get_absolute_url()`` or the related model's
`get_absolute_url()` is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this does not exist or returns a non-string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Code says: TypeError if it doesn't exist, no check for result I think. Might be worth giving it another glance and document a little more. Seems likely ppl will run into this when they just set link to True. Unlikely that get_absolute_url returns non-string. When it returns None, what happens?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If get_absolute_url does not exist -> error message: If linkify=True '{}' must have a method get_absolute_url.

The return value is used verbatim in the template, so I think that should be transparent enough?

- If a callable is passed, the returned value is used, if it's not ``None``.
- If a `dict` is passed, it's passed on to ``~django.urls.reverse``.
- If a `tuple` is passed, it must be either a (viewname, args) or (viewname, kwargs)
tuple, which is also passed to ``~django.urls.reverse``.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I also saw some checks for url and uri attributes somewhere? Not i use anymore? Let me try and find them.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I renamed it to url to align with get_absolute_url, so uri should not occur anymore.

@@ -65,7 +65,13 @@ def __init__(self, **kwargs):
for key, value in kwargs.items():
setattr(self, key, value)

def compose_url(self, record, bound_column):
def compose_url(self, **kwargs):
if hasattr(self, "uri"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this in the docs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

outdated diff...

@jieter jieter merged commit ce063d1 into master Jul 17, 2018
@jieter jieter deleted the linkify branch July 17, 2018 09:47
@jieter
Copy link
Owner Author

jieter commented Jul 17, 2018

released 2.0.0a4

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