#21 Gulp plugin #54

violetadev · 2018-09-12T17:18:51Z

This closes #21

violetadev · 2018-09-12T17:19:29Z

Please review if the code is correct. I didn't know how to implement the test based on the markdownit and markdown gulp plugins 🆘

hagenburger

Please make sure, Prettier gets the correct coding style.

I’ll think about the name and maybe come back to you.

hagenburger · 2018-09-12T19:07:09Z

plugins/gulp-pimd/index.js

@@ -0,0 +1,29 @@
+'use strict';


Remove this line. This is outdated JavaScript and not needed anymore.

hagenburger · 2018-09-12T19:08:13Z

plugins/gulp-pimd/index.js

+const pimd = require('pimd');
+
+module.exports = options => {
+	options = options || {};


Options need to be a Config object for PIMD. But as PIMD generates it automatically, no default value needs to be assigned when empty.

@artnerdnet asked me to help to understand this review.
When you look pimd/lib/document.js, Document class generate Config instance automatically when config, the second parameter is null.

class Document { constructor(source, config) { this.source = source this.config = config || new Config() ...

So, if you generate the default options value({}) there and use it as the second parameter, it will make an unexpected result than not having the second parameter.

hagenburger · 2018-09-12T19:09:39Z

plugins/gulp-pimd/package.json

+	"version": "0.0.0",
+	"description": "PIMD Gulp Plugin",
+	"license": "MIT",
+	"repository": "hagenburger/pimd/gulp-pimd",


This seems not valid.

hagenburger · 2018-09-12T19:09:50Z

plugins/gulp-pimd/package.json

+		"node": ">=6"
+	},
+	"scripts": {
+		"test": "xo && ava"


What is this?

hagenburger · 2018-09-12T19:10:25Z

plugins/gulp-pimd/readme.md

@@ -0,0 +1,27 @@
+# gulp-pimd 
+
+> Markdown to HTML with [`PIMD`](https://github.com/hagenburger/pimd)


hagenburger · 2018-09-12T19:18:56Z

plugins/gulp-pimd/test.js

@@ -0,0 +1 @@
+


Did you check this?
https://github.com/trwolfe13/gulp-markdownit/blob/master/spec/index.spec.js

@hagenburger yes but I don't understand it fully :< I looked also at markdown and the tutorial in the gulp site

hagenburger

Hey @artnerdnet, this is a first review I’ve done by reading the code. I also have to check it out locally.

hagenburger · 2018-09-26T19:24:06Z

plugins/gulp-pimd/index.js

+const { Document } = require("pimd")
+
+module.exports = options => {
+  return through.obj(function(file, enc, cb) {


Can you rename cb to callback, please? The first time I saw this years ago, it took me a while to understand cb. I would like to have the code easy to read if possible.

hagenburger · 2018-09-26T19:24:39Z

plugins/gulp-pimd/package.json

@@ -0,0 +1,36 @@
+{
+	"name": "gulp-pimd",


This indentation doesn’t follow our coding standards. Please reformat with Prettier.

hagenburger · 2018-09-26T19:25:08Z

plugins/gulp-pimd/package.json

+	"version": "0.0.0",
+	"description": "PIMD Gulp Plugin",
+	"license": "MIT",
+	"repository": "artnerdnet/pimd/plugins/gulp-pimd/",


This is not a valid repository URL. Please copy from other package.json.

hagenburger · 2018-09-26T19:25:51Z

plugins/gulp-pimd/readme.md

+npm install --save-dev gulp-pimd
+```
+
+


This indentation doesn’t follow our coding standards. Please reformat with Prettier.

hagenburger · 2018-09-26T19:26:23Z

plugins/gulp-pimd/readme.md

+
+gulp.task('default', () =>
+	gulp.src('intro.md')
+		.pipe(pimd())


pimd is not defined. Please check the code example.

hagenburger · 2018-09-26T19:27:41Z

plugins/gulp-pimd/test.js

+}
+
+describe("gulp-pimd", () => {
+  it("should render markdown if a buffer is provided.", function(done) {


“Markdown” is a name and should use title case

Remove the period (same for the next test case)

hagenburger · 2018-09-27T07:15:30Z

plugins/gulp-pimd/test.js

@@ -0,0 +1,38 @@
+const assert = require("assert")
+const File = require("vinyl")
+var gulpPimdPlugin = require(".")


Please always use const (or let if needed).

benevbright · 2018-09-29T17:27:34Z

Seems like Travis issue is fixed.
So please rebase to latest master if you had trouble because of Travis.
#70

cb to callback

var to const fixed typos

Removed typos

benevbright · 2018-10-14T18:33:43Z

plugins/gulp-pimd/index.js

+const through = require("through2")
+const PluginError = require("plugin-error")
+const { Document } = require("pimd")
+


How about we add allinc-plugin for gulp-pimd by default?
@hagenburger @artnerdnet

violetadev · 2018-10-19T10:18:29Z

@hagenburger can you check if this is good or if I should change something else?

violetadev requested review from hagenburger and benevbright September 12, 2018 17:18

violetadev added the help wanted Extra attention is needed label Sep 12, 2018

hagenburger requested changes Sep 12, 2018

View reviewed changes

benevbright added the work in progress This pull request is not ready to merge but open for feedback label Sep 23, 2018

violetadev removed help wanted Extra attention is needed work in progress This pull request is not ready to merge but open for feedback labels Sep 23, 2018

violetadev force-pushed the gulp-plugin branch from 6874656 to 45ec3d5 Compare September 26, 2018 19:18

hagenburger requested changes Sep 26, 2018

View reviewed changes

hagenburger reviewed Sep 27, 2018

View reviewed changes

gulp plugin rebase

b71f673

violetadev force-pushed the gulp-plugin branch from 79f01f9 to b71f673 Compare October 13, 2018 09:30

violetadev added 3 commits October 13, 2018 11:39

Update index.js

b91d39f

cb to callback

Update test.js

60cb3af

var to const fixed typos

Update test.js

0f2cf5f

Removed typos

benevbright reviewed Oct 14, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#21 Gulp plugin #54

#21 Gulp plugin #54

violetadev commented Sep 12, 2018

violetadev commented Sep 12, 2018

hagenburger left a comment

hagenburger Sep 12, 2018

hagenburger Sep 12, 2018

benevbright Sep 17, 2018 •

edited

Loading

hagenburger Sep 12, 2018

hagenburger Sep 12, 2018

hagenburger Sep 12, 2018

hagenburger Sep 12, 2018

violetadev Sep 13, 2018

hagenburger left a comment

hagenburger Sep 26, 2018

hagenburger Sep 26, 2018

hagenburger Sep 26, 2018

hagenburger Sep 26, 2018

hagenburger Sep 26, 2018

hagenburger Sep 26, 2018

hagenburger Sep 27, 2018

benevbright commented Sep 29, 2018 •

edited

Loading

benevbright Oct 14, 2018

violetadev commented Oct 19, 2018

		@@ -0,0 +1,27 @@
		# gulp-pimd

		> Markdown to HTML with [`PIMD`](https://github.com/hagenburger/pimd)

#21 Gulp plugin #54

Are you sure you want to change the base?

#21 Gulp plugin #54

Conversation

violetadev commented Sep 12, 2018

violetadev commented Sep 12, 2018

hagenburger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benevbright Sep 17, 2018 • edited Loading

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

hagenburger 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

Choose a reason for hiding this comment

benevbright commented Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

violetadev commented Oct 19, 2018

benevbright Sep 17, 2018 •

edited

Loading

benevbright commented Sep 29, 2018 •

edited

Loading