Huge changes #2

welcoMattic · 2016-11-21T13:56:12Z

Refacto
Replace emojis list with gitmoji
Add --init option

Ref. #3

tjoskar · 2016-11-21T17:56:23Z

Wow! This looks amazing! Thanks! I will have a look and get back to you BUT this PR will be accepted!

tjoskar · 2016-11-22T18:22:16Z

.travis.yml

+
+node_js:
+  - 6
+  - 4


Do we really need to use v4 and v6? I mean, we are going to use the same version of eslint regardless of node version, right? So if it works in v6 it is going to work in v4 and vice versa.

Yes, but as 4.x is LTS, testing on it is to be sure that we support this LTS.

Correct me if I'm wrong, but eslint will check the file statically without ever executing any code so even if eslint is running under node 4.x and returning that the code is flawless does not necessarily mean that it will work for v4.x.

What we really want is something like: https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features.md

tjoskar · 2016-11-22T18:34:38Z

.travis.yml

+  - npm install -g eslint
+
+install:
+  - npm install --only=production


Since you added a yarn.lock file, we should use yarn and according to this blog post: https://blog.travis-ci.com/2016-11-21-travis-ci-now-supports-yarn travis will always use yarn if there is a yarn.lock and we don't override the install script. So I think we should replace the command with yarn install --production.

tjoskar · 2016-11-22T18:37:00Z

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2016 Oskar Karlsson


You should add your name here. Eg, Oskar Karlsson, Mathieu Santo Stefano--Féron

tjoskar · 2016-11-22T18:40:07Z

README.md

+# Gitmoji-commit-hook

-> Add emoji to your commits
+[![Build Status](https://travis-ci.org/welcoMattic/gitmoji-commit-hook.svg?branch=master)](https://travis-ci.org/welcoMattic/gitmoji-commit-hook)


We need to update all paths after the merge. I can fix that.

tjoskar · 2016-11-22T18:55:07Z

package.json

-  "description": "Start the commit message with an applicable emoji",
-  "main": "index.js",
+  "name": "gitmoji-commit-hook",
+  "version": "1.1.0",


I think we should bump the version to 2.0.0 since the tool now requirer a internet connection AND do not ask for a issue number any more.

tjoskar · 2016-11-22T19:42:54Z

bin/index.js

+      }
+    });
+
+    fs.chmod(`${path}/prepare-commit-msg`, '755', (err) => {


How can this even work 😉. You call fs.writeFile which will create a file asynchronous but at the same time you will try to change the chmod. It works but I can't say why. I would have called fs.chmod in the callback of fs.writeFile.

tjoskar · 2016-11-22T19:52:45Z

bin/index.js

+    console.error(chalk.red('🚨  ERROR: The directory is not a git repository.'));
+  }
+} else {
+  axios.get(gitmojis)


We should cache the file so we don't are dependent of internet when we are going to commit. I'm thinking of something like this:

1. check if gitmojis.json exist on disk a. if it exist, use it but if the file is older than 3 days: try to download a new file in the background. b. if it don't exist, download the file and use it.

I will create a issue so we can fix this in the feature. We don't need to fix this now.

tjoskar · 2016-11-22T19:58:01Z

bin/index.js

+            name: gitmoji.emoji + '  ' + gitmoji.description,
+            value: gitmoji.emoji
+          };
+        })


You have removed the question about reference issue number. Was that intentionally?

Yes, I like KISS principle, so our hook aim is to add emoji to commit message. As gitmoji-cli allows to add issue number, commit title, commit message etc ... I think we dont need to ask for issue number anymore.

Far enough.

tjoskar · 2016-11-22T20:03:20Z

bin/index.js

+      });
+
+      if(/COMMIT_EDITMSG/g.test(process.argv[2])) {
+        inquirer.prompt(questions).then((answers) => {


We should return inquirer.prompt so we can catch (line 59) any errors that inquirer might give.

tjoskar · 2016-11-22T20:04:05Z

bin/index.js

+      }
+  })
+  .catch(err => {
+    console.log(err);


Should use console.error(chalk.red('🚨 ERROR: ${err}.'));

What do you think of creating a error-handler method. eg.

const errorHandler = error => { console.error(chalk.red(`🚨 ERROR: ${error}`)); process.exit(1); };

So we can handel all error message in the same manner.

tjoskar · 2016-11-22T20:18:16Z

Looking good. I just have a few question, see above.

welcoMattic · 2016-11-22T21:36:00Z

@tjoskar I just fix things after your feedback, thank you for it!

tjoskar · 2016-11-22T22:13:32Z

@welcoMattic, great but I still thinks we need to check if the file exists before we override the content, see #2 (comment)

Something like this:

if (process.argv[2] === '--init') {
  if (!pathExists.sync('.git')) {
    errorHandler('The directory is not a git repository.');
  }
  if (fileExists(${path}/prepare-commit-msg)) {
    errorHandler(`A prepare-commit hook already exists, please remove the hook (rm .git/hooks/prepare-commit-msg) or install gitmoji-commit-hook manually by adding the following content info ${${path}/prepare-commit-msg}: \nexec < /dev/tty\ngitmoji-commit-hook $1`);
  }

  fs.writeFile(${path}/prepare-commit-msg, `#!/bin/sh\nexec < /dev/tty\ngitmoji-commit-hook $1`, (err) => {
    if (err) {
      errorHandler(err);
    } else {
      fs.chmod(${path}/prepare-commit-msg, '755', (err) => {
        if (err) {
          errorHandler(err);
        } else {
          console.log(`${chalk.green('🎉  SUCCESS 🎉')}  gitmoji-commit-hook initialized with success.`);
        }
      });
    }
  });
} else {
  axios.get(gitmojis)
  ...

Or what do you think?

welcoMattic · 2016-11-23T12:19:18Z

@tjoskar Done ;)

welcoMattic mentioned this pull request Nov 21, 2016

Use gitmoji #3

Closed

welcoMattic added 18 commits November 22, 2016 00:02

🐛 Fix dependencies

bacd410

🎉 Fresh new start

36a1acd

✨ Make it works

c47fbf7

📝 Fix docs

b7640a3

✅,👷 Add eslint check

f919e54

📝 Add Travis.ci badge

faffbd0

🔧 Disable Travis.ci email notifications

052a67a

🔖 📝 Improve README.md

68482eb

📝 Add a demo gif

97babf3

📝 Add gif to README.md

006112f

📝 Add KISS principle

897a0b4

✨ Add --init option to install hook

700c80a

📝 Add doc for --init option

56e49db

💚 Fix tests

e93bdb3

🔧 Fix package.json

80d6c19

✏️ Fix name in LICENSE

96b5843

🐛 Fix hook installation

8e82a89

📝 Improve README.md structure

0a64fc1

tjoskar reviewed Nov 22, 2016

View reviewed changes

🎨,🐛 Refacto after code review by @tjoskar

57264b9

🐛,➕ Test if prepare-commit-msg hook already exists

5d06002

💚 Fix eslint install on Travis.ci

9835ce1

tjoskar merged commit 57c7e9f into tjoskar:master Nov 24, 2016

		@@ -0,0 +1,21 @@
		MIT License

		Copyright (c) 2016 Oskar Karlsson

Huge changes #2

Huge changes #2

Uh oh!

Conversation

welcoMattic commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjoskar commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjoskar commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcoMattic commented Nov 22, 2016

Uh oh!

tjoskar commented Nov 22, 2016

Uh oh!

welcoMattic commented Nov 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

welcoMattic commented Nov 21, 2016 •

edited

Loading

tjoskar commented Nov 22, 2016 •

edited

Loading