-
Notifications
You must be signed in to change notification settings - Fork 186
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
Added validate option (function) to be able to selectively filter tokens #106
Conversation
See tests for usage
@@ -32,8 +32,11 @@ function tokensToNodes(tokens, opts, doc) { | |||
|
|||
for (let i = 0; i < tokens.length; i++) { | |||
let token = tokens[i]; | |||
let validated = options.resolve(opts.validate, | |||
token.hasProtocol ? token.hasProtocol() : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would take out the hasProtocol
argument, since validation isn't necessarily related to the link protocol. If someone want that behaviour, they would just check the given token string e.g., type !== 'url' || /^(http|ftp)s?:\/\//.test(text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I just find it a pity that information obtained by the tokenizer (e.g. if there is a protocol -and which one-, the domain, the "path") is lost and unaccessible by the functions. It would be nice, in the future, to be able to access some (selected?) information about the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if the tokenizer already knows how to handle protocols, it is redundant to have to rehandle them with a regex (or something like that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that sense, I wouldn't be opposed to passing the whole token as the final argument!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much of the token interface is already exposed to the library user; assuming that "none" or "not much", then I'd rather leave it like it is in the last commit (just removing hasProtocol
) for the moment.
@fcarreiro Looks good, just a few issues |
it('Works with overriden options (validate)', function () { | ||
var options_validate = { | ||
validate: function (hasProtocol, text, type) { | ||
return type === 'email' || (hasProtocol || text.slice(0,3) === 'www'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the first check here to type !== 'url'
, since there could conceivably be tokens other than email
👍 |
Added validate option (function) to be able to selectively filter tokens
:) do you have a planned next release date? I'm wondering whether to wait for a new release and change the version in my packages file or to already use the files in /lib/ generated from master. |
@fcarreiro v2.0.0-beta.9 has been released with your updates. Huge thanks for this feature! |
In particular, it closes issue #77.
Example, the following options prevents linkifying strings like "someword.it is great"; more specifically, it only linkifies emails, strings with an explicit protocol and, otherwise, strings starting with "www".
Tests are also included/updated.