Parser refactor #43

gdolle · 2017-01-23T02:06:35Z

Major refactoring for the parser. Breaking changes (3.1.0 ?)

Changelog

Fix options bugs (random behaviour between book.json and local command, ace plugin does not support quotes in command options)
Add Url support Parsing code from URL (Feature) #42 (sync http request using sync-request modules + check url valid-url)
Refactor options using immutable maps (more robust). It makes easier future new options, see src/options.js
Support real types (not retrocompatible for book options). For example edit:true instead of edit:"true", to be more consistent (this fix an ace plugin bug).
Add parser checks for types.
Add different warn level.
template, fixlang, lang, theme are now also local/global options. We can set a template per code, a theme per code, etc.. or globally.
Refactor test for these last changes.
Add new tests for URL
End support of lang-typescript to use uniform options lang:"typescript" to be consistent with other options and to simplify functions.
Support multiple snippet codes. Now we can do [import:"tag0,tag1,..."](fixtures/test.js) to import several snippets into one codeblock , for example

//! [tag0]
int a;
//! [tag0]
// A comment or a code
//! [tag1]
int b;
//! [tag1]

will produce

int a;
int b;

It should work far better with this PR :)

I've been noticed that ace plugin might crash after a page reload in previous version, but I couldn't reproduce that problem. However, these changes fix some bugs and "might" be fix for that behaviour, we will see.

NB: It might be interesting to have a look to async operation in the future to optimize the code.

@azu waiting for your review, you'll have some work ;)

Not required in fact. `gitbook-plugin-ace` is required only if user pass "ace" or "acefull" to the template option. It is specified in the README.md.

- Add winston-color for color logs - produce a warning if gitbook-plugin-ace is not loaded (ace template won't work) - produce an error if gitbook-plugin-ace is loaded but before include-codeblock

- Should be cleaner and less error prone - Define global book option map, and possible key-value map (same as in readme). - Use immutable Map to avoid unwanted changes. - Pass `template`, `unindent` function arguments into the key-value map directly for consistency. - Systemize key-value argument for each functions.

- include-codeblock does not work if we use it inline. for example: ``` blabla [import](fixtures/test.js) blablabla ``` NB: we might have some works to add newline before and after block.

- It seems ace does work with boolean instead of strings for booleans type keys. - Update version 3.1.0, changes will be major

Options can be set globally or locally: - Set an immutable default map book.json global plugin options - Set an immutable default key-value map for local command options. - Add functions to check map types

- Refactor parseVariableFromCommand regex to be more comprehensible. - Handle boolean type - Add parse check depending on the value type and convert the parsed value to its type (JSON.parse) - Fix type check. Default l/g options map should not have `undefined` value as value type is deduced from the map.

- Update check for empty string which is not valid.

- Add default value options in package.json directly. Options map are updated from it. - Fix package default type to be consistent (avoid string). Now boolean is `true` and not `"true"`.

break backward compatibility - set hardcode lang as book option, or command option. - Remove `lang-javascript` and use standard key-val format `lang:"javascript"` or `lang:".js"`. If lang or extension (via lang) is not set, then lang is determined rom file extension. - Create template.js file for template specific codes.

issue azu#42 - Perform http GET request and retrieve url body. - Asynchronous http (module sync-request) with caching, gzip - Redirection not allowed. NB: Future optimization could be to provide async operation for the plugin.

azu · 2017-01-23T09:34:07Z

Oh, large diff..
I'll look it lator.
Can you separate refactor PR and feature PR if possible.

gdolle · 2017-01-23T13:10:22Z

src/parser.js

+ * @param {string} originalPath
+ * @return {string}
+ */
+export function getContent( filePath, originalPath )


Part of feature #42

gdolle · 2017-01-23T13:11:54Z

templates/ace-template.hbs

@@ -1,5 +1,5 @@
 {% if file.type=="asciidoc" %}++++{% endif %}
-{%ace edit="{{edit}}", check="{{check}}", theme="{{theme}}", lang="{{lang}}" %}
+{%ace edit={{edit}}, check={{check}}, theme="{{theme}}", lang="{{lang}}" %}


Major bug fix here

gdolle · 2017-01-23T13:15:20Z

@azu Actually, features are minors and deeply dependent of the of refactor. It'd be quite difficult to separate now.

Would it be ok to keep it as one PR ?

The related features changes are

multiple markers (parseVariableFromLabel => change the regex to have several comma ~1line)
URL parser ( getContent => load url or file ~5lines)

the 1 tests for each in test-parser.js

(I added comments on file changes to help)

azu · 2017-01-24T11:35:07Z

@gdolle fmm, I want to separate big PR to smll PRs by following reasons.

Add Url support #42 (sync http request using sync-request modules + check url valid-url)

Sync request is simple solution, but it is not correct solution.
Because GitBook support async hook.

https://toolchain.gitbook.com/plugins/hooks.html

(Not actuall code)

// THIS IS IMAGINE
module.exports = {
    hooks: {
        "page:before": function(page) {
            var options = this.options.pluginsConfig["include-codeblock"];
            var pageDir = path.dirname(page.rawPath);
            var parseResults = parse(page.content, pageDir, options);
            var promises = results.map(result => {
                return requestUrlAndCreateReplaceContent(result).then(newContent => {
                     replaceContent(page, newContent);
                });
            });
            return Promise.all(promises); // GitBook support async by returing promise
        }
    }
};

and

Immutable.js is overlarge for this project.
I recommened that use ES2015 Map + Poilyfill insteadof immutable.js.

Of course, Welcome to bugfix, but the bugfix should not contains other feature.
I hesitate to merge the bugfix PR.

Sorry, slow response.

gdolle · 2017-01-24T13:41:34Z

Does ES2015 Map guaranty const Map values ?

Actually refactor parser:

immutable Map for read only (more robust to my opinion)
handle key-val types (boolean, string, number). The "bug fix" is a consequence of the parser refactor (maybe my single comment is error prone) it's not really a feature.

I agree for the url feature, it is from far not perfect. I'll separate it in another PR.

azu

Does ES2015 Map guaranty const Map values ?

Yes and No.

Immutable.js is great library, but it is not first class in JavaScript.
In other word, When other constributor see the code that includes immutable.js, they have to learn about immutable.js. I think that it contains cost.

I've written some Map examples.

If you actualy want to use immutable.js, I can't prevent it.

I agree for the url feature, it is from far not perfect. I'll separate it in another PR.

Ok.

azu · 2017-01-25T08:50:57Z

src/parser.js

+    const spaces_ex = "\\s*";
+    const quotes_ex = "[\"']";
+
+    Object.keys(kv).forEach(key => {


Can ES2015 Map does same thing?

const resultMap = new Map(); kvMap.forEach(key => { // something resultMap.set(key, foo); } return resultMap;

azu · 2017-01-25T08:57:51Z

src/parser.js

+    const kv = kvMap.toObject();
+    const count = hasTitle(kv) ? codeCounter() : -1;
+    checkMapTypes( kvMap, "generatedEmbedCode" );
+    const contextMap = kvMap.concat( immutable.Map({


var mergedMap = new Map([...kvMap, ...otherMap])

var a = new Map([["k1", "v1"], ["k2", "v2"]]); var b = new Map([["k3", "v3"], ["k4", "v4"]]); var mergedMap = new Map([...a, ...b]); console.log(mergedMap); // Map { k1: "v1", k2: "v2", k3: "v3", k4: "v4" }

// Object to Map new Map(Object.entries({ k : 1 }))

azu · 2017-01-25T09:13:29Z

(Currently, this project mixed codeing style, We should apply ESLint #39)

- Remove immutable package - Remove Map object to use frozen Object instead (guaranty unmodified default map object). - Update tests.

- Remove URL parser source code (different branch + need update async) - Remove URL parser test - Remove package.json dependencies

gdolle · 2017-01-29T19:56:53Z

@azu I updated the code.

Remove immutable: I used object instead of Map and froze object that should be immutable. It should be understandable for future contributors.
Remove URL feature.

I didn't apply yet Eslint, I'll have a look into that.

See issue azu#39 - Use this convention https://github.com/textlint/textlint/blob/master/.eslintrc.js - Change Error to Warn for unused vars and allow console logs. - Add new package script : check => run eslint for javascript files in src/ and test/. (Do not add test/fixtures, test/patterns, ...). - Add travis eslint check.

gdolle · 2017-01-30T00:07:13Z

@azu I added eslint config following #39 and fix current code following the convention.
I also added a "check" script which test eslint on src/ and test/.
This script is executed in travis, therefore we can see if something is not good with respect to the coding style.
Let me know if it's ok for you like that. ;)

azu

Thanks.
I've leave some comments

azu · 2017-02-01T00:41:04Z

src/parser.js

+  // inspired from https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/string/strip.rb
+  if((s === undefined) || (s === "")) {
+return s;
+}


Can you format indent or Run $(npm bin)/eslint --fix src/ test/?

weird eslint does not fix this lines..

azu · 2017-02-01T00:42:28Z

src/parser.js

+                break;
+            default:
+                logger.error("include-codeblock: parseVariablesFromLabel: key type `" +
+                    typeof defaultKeyValueMap[key] + "` unknown (see options.js)");


add breaK; to default: block.

azu · 2017-02-01T00:44:32Z

src/parser.js

        content = removeMarkers(markerSliceCode(code, marker));
    }
-    if (unindent || keyValueObject.unindent) {
-      content = strip(content);
+    if (unindent == true) {


use === insteadof ==.

OR

const unindent = kvm.unindent !== undefined ? kvm.unindent : false; ... if (unindent) { ... } ...

azu · 2017-02-01T00:46:23Z

package.json

@@ -11,7 +11,7 @@
  "bugs": {
    "url": "https://github.com/azu/gitbook-plugin-include-codeblock/issues"
  },
-  "version": "3.0.1",
+  "version": "3.1.0",


PR is not include bump version.

azu · 2017-02-01T00:47:52Z

package.json

@@ -24,6 +24,7 @@
  },
  "scripts": {
    "build": "NODE_ENV=production babel src --out-dir lib --source-maps",
+    "check": "eslint src/*.js test/*.js",
    "watch": "babel src --out-dir lib --watch --source-maps",


Maybe, lint command is meaningful.

Also, To define "lint:fix": "eslint --fix src/*.js test/*.js" is useful.

gdolle · 2017-02-09T09:47:42Z

Sorry for the late reply. I updated the code relatively to your last review

azu · 2017-02-10T11:16:37Z

@gdolle Thanks for great work!

gdolle added 30 commits January 13, 2017 15:43

rename and add new examples per template

104f72a

Remove unrequired plugin

debeaa3

Not required in fact. `gitbook-plugin-ace` is required only if user pass "ace" or "acefull" to the template option. It is specified in the README.md.

Update readme

87d5edd

Add check plugin load order

e30c35f

- Add winston-color for color logs - produce a warning if gitbook-plugin-ace is not loaded (ace template won't work) - produce an error if gitbook-plugin-ace is loaded but before include-codeblock

update log message

e1d9726

update travis tests

0a86aba

Fix Readme

ee0e44d

use path package

04ac7ee

separate ace check

01d3544

Improve example

1b56d79

Improve custom example

ebe6feb

Improve default example

8844085

Add inline code remark

4308220

- include-codeblock does not work if we use it inline. for example: ``` blabla [import](fixtures/test.js) blablabla ``` NB: we might have some works to add newline before and after block.

Update tests

fe579c5

Remove log

0fd658c

up package version

a66beb7

Add multi snippets feature

f18a037

Update test using templates from map

ef62bcd

Add support equal char for

77835d8

Fix template quotes + up version 3.1.0 major

fa2fa54

- It seems ace does work with boolean instead of strings for booleans type keys. - Update version 3.1.0, changes will be major

Update readme

6204d57

Rename options to avoid confusion

30bd119

Separate options maps

5a9b279

Options can be set globally or locally: - Set an immutable default map book.json global plugin options - Set an immutable default key-value map for local command options. - Add functions to check map types

Clean option file

9b4352e

Move convertValue

f78b4ff

Update marker check with empty string

a2bc0af

- Update check for empty string which is not valid.

Update package options

8018e28

- Add default value options in package.json directly. Options map are updated from it. - Fix package default type to be consistent (avoid string). Now boolean is `true` and not `"true"`.

Revert options get

9aab0f5

gdolle added 6 commits January 22, 2017 16:21

merge from upstream

65dc7fc

Merge branch 'master' into parser-refactor

f144ddc

Add support of URL (sync)

d1d741b

issue azu#42 - Perform http GET request and retrieve url body. - Asynchronous http (module sync-request) with caching, gzip - Redirection not allowed. NB: Future optimization could be to provide async operation for the plugin.

Update readme

a4fe4b7

Update readme

8ee01db

azu self-requested a review January 23, 2017 09:34

gdolle commented Jan 23, 2017

View reviewed changes

azu reviewed Jan 25, 2017

View reviewed changes

gdolle added 2 commits January 29, 2017 19:44

Remove immutable.js

188cf07

- Remove immutable package - Remove Map object to use frozen Object instead (guaranty unmodified default map object). - Update tests.

Remove parser feature

57a57e3

- Remove URL parser source code (different branch + need update async) - Remove URL parser test - Remove package.json dependencies

gdolle added 2 commits January 30, 2017 00:51

add eslint config

26d95fa

azu requested changes Feb 1, 2017

View reviewed changes

Update reviewed code

627f731

azu approved these changes Feb 10, 2017

View reviewed changes

azu merged commit ce64163 into azu:master Feb 10, 2017

This was referenced Feb 10, 2017

Introduce ESLint #39

Closed

Constantly got "Error: Configuration Error: pluginsConfig.include-codeblock.unindent is not of a type(s) boolean" #44

Open

azu mentioned this pull request May 14, 2017

Parsing code from URL (Feature) #42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser refactor #43

Parser refactor #43

gdolle commented Jan 23, 2017

azu commented Jan 23, 2017

gdolle Jan 23, 2017

gdolle Jan 23, 2017

gdolle commented Jan 23, 2017

azu commented Jan 24, 2017 •

edited

Loading

gdolle commented Jan 24, 2017

azu left a comment

azu Jan 25, 2017

azu Jan 25, 2017

azu commented Jan 25, 2017

gdolle commented Jan 29, 2017

gdolle commented Jan 30, 2017 •

edited

Loading

azu left a comment

azu Feb 1, 2017

gdolle Feb 9, 2017

azu Feb 1, 2017

azu Feb 1, 2017

azu Feb 1, 2017

azu Feb 1, 2017

gdolle commented Feb 9, 2017

azu commented Feb 10, 2017

Parser refactor #43

Parser refactor #43

Conversation

gdolle commented Jan 23, 2017

azu commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdolle commented Jan 23, 2017

azu commented Jan 24, 2017 • edited Loading

gdolle commented Jan 24, 2017

azu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu commented Jan 25, 2017

gdolle commented Jan 29, 2017

gdolle commented Jan 30, 2017 • edited Loading

azu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdolle commented Feb 9, 2017

azu commented Feb 10, 2017

azu commented Jan 24, 2017 •

edited

Loading

gdolle commented Jan 30, 2017 •

edited

Loading