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

Pass links through (a) callback(s) #56

Closed
jsocol opened this issue Feb 17, 2012 · 4 comments
Closed

Pass links through (a) callback(s) #56

jsocol opened this issue Feb 17, 2012 · 4 comments
Labels
Milestone

Comments

@jsocol
Copy link
Contributor

jsocol commented Feb 17, 2012

An interesting idea from @aehlke in pull req #43 is to pass links through a callback or callbacks that accept and return (possibly among other things) a dict of attributes.

I think this has a lot of potential to simplify the linkify API and address the issues that delinkify aimed to, but in a better way.

Total straw man API might be:

def linkify(text, callback):
    # ... snip ...

def some_callback(text, attributes=None, new=False):  # This signature would be specified in docs.
    # Do processing, including things like url and text munging that are currently callbacks.
    # Create a link.
   return {'href': href, ...}  # Return a dict, everything's OK.
    # Or don't create a link/remove the link.
   return None  # Existing links removed, new links not created.

The obvious question that arises is what to do with multiple callbacks. It'd be great to make these callback functions small, atomic, etc, so something along the lines of Django context processors seems obvious:

def linkify(text, callbacks=None):
    if callbacks is None:
        callbacks = []
    elsif callable(callbacks):
        callbacks = [callbacks]
    # ... snip ...
    # Found a link!
    attrs = {'some': 'dict', ...}
    for cb in callbacks:
        a = cb(link_text, attrs, new)
        if a is None:
            # Break out of this somehow, just replace everything with the text.
        attrs.update(a)
    # Render the link with the attributes in attrs

Pros of this approach:

  • Generalizes everything linkify currently does. filter_url and filter_text are already callbacks, so they could easily be updated to return dicts. Adding or setting attributes like rel and target is trivial, etc.
  • Is extremely flexible.
  • It's easy to create a bunch of default callbacks for people to use.
  • It can handle what delinkify was meant to do--just write a callback that only allows relative links or links from certain domains.
  • It can handle the skip_existing_links idea from pull req Added a flag to linkify to skip existing links #43.

Cons, and some suggestions:

  • Adding new callbacks requires repeating all the old ones. Hopefully this is solvable (e.g. linkify(text, DEFAULT_CALLBACKS + [my_callback]) or something).
  • The order of the callbacks is critical and might introduce subtle bugs for people? (They should have tests.)
  • It's flexible but more complicated for developers.
  • How to deal with the link text? (Maybe add it to the dict as _text? Something that can never be an HTML attribute.)
  • As described above, wouldn't ever remove attributes unless their was some sort of whitelist/blacklist filter callback. (Maybe allow setting 'some_attr': None to remove it? Or make sure you return the whole attr dict, unlike Django context processors.)
@kumar303
Copy link

We use filter_url on AMO to add the outgoing proxy. This new callback style looks good to me.

@jsocol
Copy link
Contributor Author

jsocol commented Feb 18, 2012

The AMO filter_url callback would need to be rewritten to set the 'href' value in the attributes dict and return the dict. And calls to linkify would need to be modified.

@jsocol
Copy link
Contributor Author

jsocol commented Feb 18, 2012

I realize that my last con is just as much an issue in the current implementation. While rel and target can get set, and href can get munged, any other attributes won't be touched (on existing links). So, this actually adds the ability to add an attribute filter if you want, assuming we make each callback return the whole dict.

@jsocol
Copy link
Contributor Author

jsocol commented Feb 27, 2012

Going to dive into implementation on this tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants