Skip to content

Added ftp urls to be catched in urlize#587

Closed
choialdrichdy wants to merge 2 commits intopallets:masterfrom
choialdrichdy:master
Closed

Added ftp urls to be catched in urlize#587
choialdrichdy wants to merge 2 commits intopallets:masterfrom
choialdrichdy:master

Conversation

@choialdrichdy
Copy link
Copy Markdown

using #522 as an inspiration, i have added the 'ftp://' url to be accepted in urlize. Also, i have separated both the domain and schemes in a list.


If target is not None, a target attribute will be added to the link.
"""
scheme = ['http://','https://','ftp://']

This comment was marked as off-topic.

This comment was marked as off-topic.

@ThiefMaster
Copy link
Copy Markdown
Member

Commit messages should use present tense / command style, e.g,. "Add ftp urls ..." instead of "Added ...". Also, you could squash the second commit into the first one - the fact that you fixed the tests in the first commit is not really something useful in the project history in case the PR gets merged.

@ThiefMaster
Copy link
Copy Markdown
Member

Would it make sense to have only http/https by default and add an extra_schemes argument if you want to include stuff like ftp? I think ftp URLs are not that common after all.

@mitsuhiko
Copy link
Copy Markdown
Contributor

We recently added a policy configuration. I'm happy to accept a policy for the url schemes supported. I'm not super happy with the urlize filter as it stands right now because it's not super correct. Instead of piling up on it i rather accept a more generalized PR that improves on it (eg: tld handling etc.).

@mitsuhiko mitsuhiko closed this Jan 6, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants