Add commitizen support #122

LinusU · 2016-03-24T16:21:22Z

lijunle · 2016-03-24T16:23:15Z

src/special/commitizen.js

@@ -0,0 +1,22 @@
+import path from 'path';
+
+export default function parseFerossStandard(content, filePath, deps, rootDir) {


Change the function name parseFerossStandard to parseCommitizen.

Ooops :) will fix

lijunle · 2016-03-24T16:26:04Z

It seems ESLint is broken again. 😢

Could you try run npm run lint offline?

Update: I get this:

Peer eslint-config-airbnb@6.2.0 wants eslint@^2.4.0

LinusU · 2016-03-24T16:27:37Z

I tried but it was broken locally as well :/

In addition, four unrelated tests are failing on my local machine:

  4 failing

  1) depcheck command line should support Typescript syntax:

      AssertionError: expected Array [ 'react', 'ts-dep-1', 'ts-dep-2', 'unused-dep' ] to equal Array [ 'unused-dep' ] (at length, A has 4 and B has 1)
      + expected - actual

       [
      -  "react"
      -  "ts-dep-1"
      -  "ts-dep-2"
         "unused-dep"
       ]

      at Assertion.fail (node_modules/should/lib/assertion.js:92:17)
      at Assertion.Object.defineProperty.value (node_modules/should/lib/assertion.js:164:19)
      at cli.js:68:36
      at run (node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:89:22)
      at node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:102:28
      at flush (node_modules/babel-polyfill/node_modules/core-js/modules/_microtask.js:14:5)

  2) depcheck command line should support SASS/SCSS syntax:

      AssertionError: expected Array [ 'sass-dep', 'scss-dep', 'unused-sass-dep' ] to equal Array [ 'unused-sass-dep' ] (at length, A has 3 and B has 1)
      + expected - actual

       [
      -  "sass-dep"
      -  "scss-dep"
         "unused-sass-dep"
       ]

      at Assertion.fail (node_modules/should/lib/assertion.js:92:17)
      at Assertion.Object.defineProperty.value (node_modules/should/lib/assertion.js:164:19)
      at cli.js:68:36
      at run (node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:89:22)
      at node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:102:28
      at flush (node_modules/babel-polyfill/node_modules/core-js/modules/_microtask.js:14:5)

  3) depcheck should support Typescript syntax:

      AssertionError: expected Array [ 'react', 'ts-dep-1', 'ts-dep-2', 'unused-dep' ] to equal Array [ 'unused-dep' ] (at length, A has 4 and B has 1)
      + expected - actual

       [
      -  "react"
      -  "ts-dep-1"
      -  "ts-dep-2"
         "unused-dep"
       ]

      at Assertion.fail (node_modules/should/lib/assertion.js:92:17)
      at Assertion.Object.defineProperty.value (node_modules/should/lib/assertion.js:164:19)
      at index.js:37:36
      at run (node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:89:22)
      at node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:102:28
      at flush (node_modules/babel-polyfill/node_modules/core-js/modules/_microtask.js:14:5)

  4) depcheck should support SASS/SCSS syntax:

      AssertionError: expected Array [ 'sass-dep', 'scss-dep', 'unused-sass-dep' ] to equal Array [ 'unused-sass-dep' ] (at length, A has 3 and B has 1)
      + expected - actual

       [
      -  "sass-dep"
      -  "scss-dep"
         "unused-sass-dep"
       ]

      at Assertion.fail (node_modules/should/lib/assertion.js:92:17)
      at Assertion.Object.defineProperty.value (node_modules/should/lib/assertion.js:164:19)
      at index.js:37:36
      at run (node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:89:22)
      at node_modules/babel-polyfill/node_modules/core-js/modules/es6.promise.js:102:28
      at flush (node_modules/babel-polyfill/node_modules/core-js/modules/_microtask.js:14:5)

lijunle · 2016-03-24T16:30:21Z

src/special/commitizen.js

+    const metadata = JSON.parse(content);
+
+    if (metadata.config && metadata.config.commitizen && metadata.config.commitizen.path) {
+      const {path} = metadata.config.commitizen;


I think you will get ESLint warnings here, because path is already defined from import path from 'path'. Could you please wait and rebase on my ESLint fix? We should ensure ESLint from CI build.

Ahh, true.

Of course, no stress 👍

lijunle · 2016-03-24T16:42:42Z

src/special/commitizen.js

+    if (metadata.config && metadata.config.commitizen && metadata.config.commitizen.path) {
+      const commitizenPath = metadata.config.commitizen.path;
+
+      if (commitizenPath.startsWith('./node_modules/')) {


The separator (/) is OS specific and bad.

You could do this:

const resolvedPluginPath = require.resolve(commitizenPath); // full path const relatedPluginPath = path.relative(rootDir, resolvedPluginPath); // *maybe* node_module relative path if (relatedPluginPath.startsWith('node_module')) { const normalizedPluginPath = relativePluginPath.substring('node_module/').replace(/\\/g, '/'); const pluginName = requirePackageName(normalizedPluginPath); // require-package-name will handle the scoped package // finally, we get the plugin name here... }

lijunle · 2016-03-24T16:46:45Z

@LinusU Thanks for sending PR, please resolve the comments. I am resolving the ESLint failure in #123. Once that is merged, please rebase your branch to newest one and squash your commits. Great thanks! 😸

lijunle · 2016-03-24T17:33:53Z

@LinusU Done. Could you please rebase your changes on the new master?

Besides, with the default npm i, I think you are getting some default failing test cases. Those will be passed with peer dependencies installed. I am resolving that problem. However, that will not block your merge.

Thanks! :D

LinusU · 2016-03-24T19:31:08Z

Absolutely, I'm on it 👍

btw, I could install them locally, which modules is it?

lijunle · 2016-03-24T19:32:34Z

npm install typescript node-sass

codecov-io · 2016-03-24T19:37:07Z

Current coverage is `96.12%`

Merging #122 into master will decrease coverage by -0.32% as of 6c6b9df

@@            master    #122   diff @@
======================================
  Files           22      23     +1
  Stmts          422     439    +17
  Branches        81      85     +4
  Methods          0       0       
======================================
+ Hit            407     422    +15
- Partial          8       9     +1
- Missed           7       8     +1

Review entire Coverage Diff as of 6c6b9df

Powered by Codecov. Updated on successful CI builds.

LinusU · 2016-03-24T19:40:09Z

Great, now the tests works!

However, I'm having some problems with the code snippet you suggested. require.resolve will only work if the packet is actually installed, is that really what we want?

Maybe I'm missing something...

Besides these two cases, the user may write full OS path inside the box too. My above code snippet will handle that.

But if they write a full path, it's probably not a module? Maybe it would be best to just only support the plain syntax (only package name). We don't want to parse all possible values for the config, just find out wether or not it's a module that is used.

Thoughts?

LinusU · 2016-03-24T19:44:35Z

For reference, when using require.resolve, this error is thrown:

Error: Cannot find module './node_modules/cz-test'

Also, won't require.resolve be relative to the file I'm in (src/special/commitizen.js) and then always fail when it starts with ./?

lijunle · 2016-03-24T19:46:35Z

However, I'm having some problems with the code snippet you suggested. require.resolve will only work if the packet is actually installed, is that really what we want?

Yes, we did the same thing for SASS parser. Production code is here. Simple test fixtures are created here.

Error: Cannot find module './node_modules/cz-test'

Just create a simple package (with package.json and index.js) to test/special/node_module folder. This folder has reserved from git ignore.

LinusU · 2016-03-24T19:50:13Z

Production code is here.

I don't see require.resolve being used here?

LinusU · 2016-03-24T19:51:04Z

require-package-name looks awesome though 🙌

lijunle · 2016-03-24T20:04:26Z

I don't see require.resolve being used here?

Sorry, you should not use require.resolve. I think two cases for module name and relative path (start with '.') is fine.

I am not sure how to handle node_module/plugin/index.js. May it is not supported.

lijunle · 2016-03-24T20:07:43Z

If the value starts with '.', path.resolve(rootpath, value), then relative, normalize, require package name.

LinusU · 2016-03-24T20:13:24Z

Got it now :)

node_module/plugin/index.js is supported, as well as referring to any other file within a module.

I think I have a awesome solution, submitting it now 🙌

lijunle · 2016-03-25T01:51:37Z

Merged, thanks! 👍

LinusU · 2016-03-25T10:18:31Z

Thank you 🎉

lijunle reviewed Mar 24, 2016
View reviewed changes

LinusU force-pushed the commitizen branch from a455bf1 to 4cf5b32 Compare March 24, 2016 16:25

lijunle reviewed Mar 24, 2016
View reviewed changes

LinusU force-pushed the commitizen branch from 4cf5b32 to ab909cb Compare March 24, 2016 16:32

lijunle reviewed Mar 24, 2016
View reviewed changes

lijunle added the feature label Mar 24, 2016

lijunle added this to the 0.6.2 milestone Mar 24, 2016

LinusU force-pushed the commitizen branch from ab909cb to 23ba726 Compare March 24, 2016 19:32

Add commitizen support

fa94fa8

LinusU force-pushed the commitizen branch from 23ba726 to fa94fa8 Compare March 24, 2016 20:13

lijunle merged commit 53124f0 into depcheck:master Mar 25, 2016

lijunle mentioned this pull request Mar 25, 2016

Support for commitizen modules #119

Closed

LinusU deleted the commitizen branch March 25, 2016 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commitizen support #122

Add commitizen support #122

LinusU commented Mar 24, 2016

lijunle Mar 24, 2016

LinusU Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle Mar 24, 2016

LinusU Mar 24, 2016

lijunle Mar 24, 2016

lijunle commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

codecov-io commented Mar 24, 2016

LinusU commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 25, 2016

LinusU commented Mar 25, 2016

		@@ -0,0 +1,22 @@
		import path from 'path';

		export default function parseFerossStandard(content, filePath, deps, rootDir) {

Add commitizen support #122

Add commitizen support #122

Conversation

LinusU commented Mar 24, 2016

lijunle Mar 24, 2016

Choose a reason for hiding this comment

LinusU Mar 24, 2016

Choose a reason for hiding this comment

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle Mar 24, 2016

Choose a reason for hiding this comment

LinusU Mar 24, 2016

Choose a reason for hiding this comment

lijunle Mar 24, 2016

Choose a reason for hiding this comment

lijunle commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

codecov-io commented Mar 24, 2016

Current coverage is 96.12%

LinusU commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 24, 2016

lijunle commented Mar 24, 2016

LinusU commented Mar 24, 2016

lijunle commented Mar 25, 2016

LinusU commented Mar 25, 2016

Current coverage is `96.12%`