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

Merged
merged 2 commits into from Jan 22, 2016

Conversation

Projects
None yet
2 participants
@fcarreiro

fcarreiro commented Jan 22, 2016

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".

var options = {
    validate: function (text, type) {
        return type !== 'url' || /^(http|ftp)s?:\/\//.test(text) || text.slice(0,3) === 'www';
    }
};

Tests are also included/updated.

@@ -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,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

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)

This comment has been minimized.

@fcarreiro

fcarreiro Jan 22, 2016

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.

This comment has been minimized.

@fcarreiro

fcarreiro Jan 22, 2016

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).

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

In that sense, I wouldn't be opposed to passing the whole token as the final argument!

This comment has been minimized.

@fcarreiro

fcarreiro Jan 22, 2016

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.

@@ -77,6 +77,10 @@ function linkifyChars(str, opts) {
for (var i = 0; i < tokens.length; i++) {
let token = tokens[i];
let validated = linkify.options.resolve(opts.validate,
token.hasProtocol ? token.hasProtocol() : false,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

Same here, take out hasProtocol argument

@@ -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,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

I would add a short circuit guard here, so that we don't check on every token (i.e., generic text tokens)

let validated = token.isLink && options.resolve(opts.validate ...
@@ -77,6 +77,10 @@ function linkifyChars(str, opts) {
for (var i = 0; i < tokens.length; i++) {
let token = tokens[i];
let validated = linkify.options.resolve(opts.validate,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

Same comment here about the short-circuit guard

@@ -37,11 +37,16 @@ function linkifyStr(str, opts={}) {
for (let i = 0; i < tokens.length; i++) {
let token = tokens[i];
if (token.isLink) {
let validated = options.resolve(opts.validate,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

+1 short circuit guard

@@ -37,11 +37,16 @@ function linkifyStr(str, opts={}) {
for (let i = 0; i < tokens.length; i++) {
let token = tokens[i];
if (token.isLink) {
let validated = options.resolve(opts.validate,
token.hasProtocol ? token.hasProtocol() : false,

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

+1 remove protocol argument

// Test specific options
it('Works with overriden options (validate)', function () {
var options_validate = {

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

Can you change these variables to camelCase?

@nfrasser

This comment has been minimized.

Collaborator

nfrasser commented Jan 22, 2016

@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');

This comment has been minimized.

@nfrasser

nfrasser Jan 22, 2016

Collaborator

I would change the first check here to type !== 'url', since there could conceivably be tokens other than email

@nfrasser

This comment has been minimized.

Collaborator

nfrasser commented Jan 22, 2016

👍

nfrasser added a commit that referenced this pull request Jan 22, 2016

Merge pull request #106 from fcarreiro/master
Added validate option (function) to be able to selectively filter tokens

@nfrasser nfrasser merged commit e7ac584 into SoapBox:master Jan 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 97.808%
Details
@fcarreiro

This comment has been minimized.

fcarreiro commented Jan 22, 2016

:) 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.

@nfrasser

This comment has been minimized.

Collaborator

nfrasser commented Jan 29, 2016

@fcarreiro v2.0.0-beta.9 has been released with your updates. Huge thanks for this feature!

@nfrasser nfrasser referenced this pull request Jan 29, 2016

Closed

It kills css import src #78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment