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.

Sign up for GitHub

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

Jump to bottom

Allow using of non-word characters in [import] tag #52

Merged
merged 8 commits into from Jul 13, 2017
Merged

Allow using of non-word characters in [import] tag #52

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6b7995a
Allow more characters to be used in string fields of [include] tag
lwchkg Jul 11, 2017
4d052cd
Removed unwanted file.
lwchkg Jul 11, 2017
a5ecb5c
Respond to comments in pull request #68
lwchkg Jul 13, 2017
8e4a45a
Fix errors in test, fix typo, add license for unescape-string.js
lwchkg Jul 13, 2017
cfd83fb
Update dependency of gitbook-cli in example builds
lwchkg Jul 13, 2017
4c231e9
Respond to comments again. (Did not read a comment correctly)
lwchkg Jul 13, 2017
2fb9165
Fix missing empty line
lwchkg Jul 13, 2017
89bc9ee
Changed the last assert.equal to assert.strictEqual
lwchkg Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -89,6 +89,7 @@
}
},
"dependencies": {
"commonmark": "0.27.0",
"language-map": "^1.1.1",
"handlebars": "^4.0.5",
"winston-color": "^1.0.0"
Expand Down
124 changes: 75 additions & 49 deletions src/parser.js
View file
Open in desktop
Expand Up @@ -3,13 +3,25 @@
const path = require("path");
const Handlebars = require("handlebars");
const logger = require("winston-color");
import { defaultKeyValueMap, initOptions, checkMapTypes, convertValue } from "./options.js";
const common = require("commonmark/lib/common.js");
Copy link
Owner

Choose a reason for hiding this comment

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

Does commonmark export this? ( Is it a undocument API? )
We want to avoid to use direct require.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not documented, nor exported by commonmark package.
I'll copy the implementation to our own code (and the license too - which appears to be 2-clause BSD).

import { defaultKeyValueMap, initOptions, checkMapTypes } from "./options.js";
import { getLang } from "./language-detection";
import { getMarker, hasMarker, markerSliceCode, removeMarkers } from "./marker";
import { sliceCode, hasSliceRange, getSliceRange } from "./slicer";
import { hasTitle } from "./title";
import { getTemplateContent, readFileFromPath } from "./template";
const markdownLinkFormatRegExp = /\[([^\]]*?)\]\(([^\)]*?)\)/gm;
const markdownLinkFormatRegExp = /\[((?:[^\]]|\\.)*?)\]\(((?:[^\)]|\\.)*?)\)/gm;

const keyEx = "\\w+";
const kvsepEx = "[:=]";
const spacesEx = "\\s*";
const quoteEx = "[\"']";
const valEx = "(?:[^'\"\\\\]|\\\\.)*";
const argEx = `${quoteEx}${valEx}${quoteEx}|true|false`;
const expressionEx = `(${keyEx})${kvsepEx}${spacesEx}(${argEx})`;
const expressionRegExp = new RegExp(expressionEx, "g");

const markerRegExp = /^\s*(([-\w\s]*,?)*)$/;

/**
* A counter to count how many code are imported.
Expand Down Expand Up @@ -68,6 +80,47 @@ export function containIncludeCommand(commands = []) {
});
}

/**
* Parse the given value to the given type. Returns the value if valid, otherwise returns undefined.
* @param {string} value
* @param {string} type "string", "boolean"
* @param {string} key
* @return {boolean|string|undefined}
*/
export function parseValue(value, type, key) {
switch (type) {
case "string":
value = common.unescapeString(value.substring(1, value.length - 1));
Copy link
Owner

Choose a reason for hiding this comment

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

const unescapedValue = ... is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

if (key === "marker" && !markerRegExp.test(value)) {
logger.error(
"include-codeblock: parseVariablesFromLabel: invalid value " +
`\`${value}\` in key \`marker\``
);
return undefined;
}
return value;

case "boolean":
if (["true", '"true"', "'true'"].includes(value)) {
return true;
}

if (["false", '"false"', "'false'"].includes(value)) {
return false;
}

logger.error(
"include-codeblock: parseVariablesFromLabel: invalid value " +
`\`${value}\` in key \`${key}\`. Expect true or false.`
);
return undefined;
}
logger.error(
`include-codeblock: parseVariablesFromLabel: unknown key type \`${type}\` (see options.js)`
);
return undefined;
}

/** Parse the command label and return a new key-value object
* @example
* [import,title:"<thetitle>",label:"<thelabel>"](path/to/file.ext)
Expand All @@ -77,56 +130,29 @@ export function containIncludeCommand(commands = []) {
*/
export function parseVariablesFromLabel(kvMap, label) {
const kv = Object.assign({}, kvMap);
const beginEx = "^.*";
const endEx = ".*$";
const sepEx = ",?";
const kvsepEx = "[:=]";
const spacesEx = "\\s*";
const quotesEx = "[\"']";

Object.keys(kv).forEach(key => {
let keyEx = "(" + key + ")";
let valEx = "([-\\w\\s]*)";
if (key === "marker") {
keyEx = "(import|include)";
valEx = "(([-\\w\\s]*,?)*)";

let match = "";
while ((match = expressionRegExp.exec(label))) {
let key = match[1];
if (["include", "import"].includes(key)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Node.js v4 does not support Array#inclues
http://node.green/#ES2016-features-Array-prototype-includes-Array-prototype-includes
We want to support Node.js LTS as possible.

Can you use indexOf or add ponyfill or polyfill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be handled by babel instead of by us? I mean, you can use "babel-preset-env", and specify the node version in .babelrc

BTW, I was told that the gitbook server used node 5.1.*. But recently the broken gitbook-cli 2.3.1 moved to node LTS (currentlt v6).

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be handled by babel instead of by us?

No. Babel only transpile syntax by default.

I think that this PR fix a bug - it is a patch version update.
Dropping Node.js 4.x make breaking change - it is a major version update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your clarification! Done.

key = "marker";
}
// Add value check here
switch (typeof defaultKeyValueMap[key]) {
case "string":
valEx = quotesEx + valEx + quotesEx;
break;
case "boolean":
// no quotes
valEx = quotesEx + "?(true|false)" + quotesEx + "?";
break;
default:
logger.error(
"include-codeblock: parseVariablesFromLabel: key type `" +
typeof defaultKeyValueMap[key] +
"` unknown (see options.js)"
);
break;
const value = match[2];

if (!kv.hasOwnProperty(key)) {
logger.error(
"include-codeblock: parseVariablesFromLabel: unknown key " +
`\`${key}\` (see options.js)`
);
return;
}
// Val type cast to string.
const regStr =
beginEx +
sepEx +
spacesEx +
keyEx +
spacesEx +
kvsepEx +
spacesEx +
valEx +
spacesEx +
sepEx +
endEx;
const reg = new RegExp(regStr);
const res = label.match(reg);
if (res) {
kv[key] = convertValue(res[2], typeof defaultKeyValueMap[key]);

const parsedValue = parseValue(value, typeof defaultKeyValueMap[key], key);
if (parsedValue !== undefined) {
kv[key] = parsedValue;
}
});
}

return Object.freeze(kv);
}

Expand Down
71 changes: 70 additions & 1 deletion test/parser-test.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ import {
containIncludeCommand,
splitLabelToCommands,
strip,
parseValue,
parseVariablesFromLabel
} from "../src/parser";

Expand Down Expand Up @@ -45,7 +46,45 @@ describe("parse", function() {
assert(containIncludeCommand(commands));
});
});
context("parseVariablesFromLabel ", function() {
describe("parseValue", function() {
it("should unescape string parameter", function() {
const result = parseValue(
'"\\!\\"\\#\\$\\%\\&\\\'\\(\\)\\*\\+\\,\\-\\.\\/' +
"\\:\\;\\<\\=\\>\\?\\@\\[\\\\\\]\\^\\_\\`\\{\\|\\}\\~" +
'&amp;&lt;&gt;&#65;&#x41;"',
"string",
""
);
assert.equal(result, "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~&<>AA");
});
it("should backslash unescape commonmark defined characters only", function() {
const result = parseValue('"\\r\\n\\[\\]"', "string", "");
assert.equal(result, "\\r\\n[]"); // \r and \n should not be unescaped.
});
it("should validate markers", function() {
let result = parseValue('" marker0 , marker1 "', "string", "marker");
assert.equal(result, " marker0 , marker1 ");

result = parseValue('"~invalid~"', "string", "marker");
assert.equal(result, undefined);
Copy link
Owner

Choose a reason for hiding this comment

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

use strictEqual insteadof equal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted with thanks.

});
it("should parse boolean values", function() {
let result = parseValue("true", "boolean", "");
assert.equal(result, true);
result = parseValue('"true"', "boolean", "");
assert.equal(result, true);
result = parseValue("'true'", "boolean", "");
assert.equal(result, true);

result = parseValue("false", "boolean", "");
assert.equal(result, false);
result = parseValue('"false"', "boolean", "");
assert.equal(result, false);
result = parseValue("'false'", "boolean", "");
assert.equal(result, false);
});
});
describe("parseVariablesFromLabel ", function() {
it("should retrieve edit boolean", function() {
const resmap = parseVariablesFromLabel(kvmap, "include,edit:true");
const results = resmap;
Expand Down Expand Up @@ -113,6 +152,36 @@ describe("parse", function() {
const results = resmap;
assert.equal(results.marker, "");
});
it("should handle characters for string parameter", function() {
const resmap = parseVariablesFromLabel(
kvmap,
'import,title="test+with-special*string"'
);
const results = resmap;
assert.equal(results.title, "test+with-special*string");
});
it("should unescape string parameter", function() {
const resmap = parseVariablesFromLabel(
kvmap,
'import,title="\\!\\"\\#\\$\\%\\&\\\'\\(\\)\\*\\+\\,\\-\\.\\/' +
"\\:\\;\\<\\=\\>\\?\\@\\[\\\\\\]\\^\\_\\`\\{\\|\\}\\~" +
'&amp;&lt;&gt;&#65;&#x41;"'
);
const results = resmap;
assert.equal(results.title, "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~&<>AA");
});
it("should backslash unescape commonmark defined characters only", function() {
const resmap = parseVariablesFromLabel(kvmap, 'import,title="\\r\\n\\[\\]"');
const results = resmap;
assert.equal(results.title, "\\r\\n[]"); // \r and \n should not be unescaped.
});
it("should not parse string argument into another key-value pair", function() {
assert.equal(kvmap.edit, false);
const resmap = parseVariablesFromLabel(kvmap, 'import,title="edit:true"');
const results = resmap;
assert.equal(results.edit, false);
assert.equal(results.title, "edit:true");
});
});
// inspired from https://github.com/rails/rails/blob/master/activesupport/test/core_ext/string_ext_test.rb
describe("strip", function() {
Expand Down