Feat/sgcrc by aichbauer · Pull Request #3 · JPeer264/node-semantic-git-commit-cli

aichbauer · 2017-03-14T18:03:17Z

Added all types that we discussed.

Add: Files added
Chore: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm) and moving files
CI: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
Docs: Documentation only changes
Feat: A new feature
Fix: A bug fix
Perf: A code change that improves performance
Refactor: A code change that neither fixes a bug nor adds a feature
Style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
Test: Adding missing tests or correcting existing tests

coveralls · 2017-03-14T18:06:19Z

Coverage increased (+7.8%) to 88.889% when pulling a7d3108 on feat/sgcrc into d2e2779 on master.

aichbauer · 2017-03-14T18:18:05Z

Do we need a type Add:? I think we should always use a more meaningful type.

JPeer264

What do prefer over Add:?

JPeer264 · 2017-03-14T19:19:40Z

lib/getConfig.js

  let configObject = json.readToObjSync(pathString);

-  if (!configObject) {
+  if (configObject) {


This should be !configObject. If altPath or cwd/.sgcrc does not exist it should read the package.json

good point, how about:

let package = json.readToObjSync(path.join(cwd, 'package.json')); configObject = typeof package === 'object' ? package.sgc : package;

JPeer264 · 2017-03-14T19:24:17Z

package.json

+      {
+        "emoji": ":white_check_mark:",
+        "prefix": "Test:",
+        "description": "Adding missing tests or correcting existing tests"


package.json and test/fixtures/ .sgcrc just need one or two "types" - these are just for testing, so we can keep it small 😊

.sgcrc_default are the default types the user actually see, if there is no .sgcrc or (package.json).sgc available - so this is fine

Yes that is true.

How can we test the second if stmt for branch coverage?

If there is a sgc, present in the package.json we never get into the second stmt.

I thought about the same, the only thing I thought of is to remove the sgc in package.json temporarily.

In the tests it could be like following:

copy package.json to package.json.back

modify the original package.json

test

rm original package.json

mv package.json.back to package.json

I think we should temporary add the sgc in the package.json. Because by default it should use the .sgcrc_default and not only one type we just have in the package.json for testing purposes.

It will always use .sgcrc_default as default if the user did not specify any config files - the difference is process.cwd() and __dirname. But we can add it temporarily - which would be easier to manage.

Easier because we will take the config from test/fixtures/.sgcrc and add it to the package.json object. So there will be just one file to change if there would be an issue. 👍

aichbauer · 2017-03-14T20:11:40Z

Files added is not really meaningful.

All of the time when you add files you do this because you add docs, features, or some of the other types.

JPeer264 · 2017-03-14T20:30:06Z

Yap exactly: we got Feat: instead. We can change that

coveralls · 2017-03-14T22:44:41Z

Coverage decreased (-7.06%) to 78.125% when pulling 7c08852 on feat/sgcrc into b97541b on master.

JPeer264 · 2017-03-14T23:00:32Z

test/getConfig.js

+    }).then(() => {
+      // restore package.json from package.json.back
+      fs.rename(cwd + '/package.json.back', cwd + '/package.json');
+    });


Quite confusing for a test with a quite long promise chain. I would suggest to use fs-extra and use its sync methods, as test perfomance is not an issue right now but readability is. Methods such as: moveSync(), writeFileSync() (not fs-extra), removeSync() , ...

fs-extra

JPeer264 · 2017-03-15T05:59:51Z

test/getConfig.js

-      fs.rename(cwd + '/package.json.back', cwd + '/package.json');
-    });
+  fs.createReadStream(cwd + '/package.json')
+    .pipe(fs.createWriteStream(cwd + '/package.json.back'));


I know I am picky 😅, but this can be shortened to:

fs.copySync(`${cwd} /package.json`, `${cwd} /package.json.back`);

JPeer264 · 2017-03-15T06:06:22Z

test/getConfig.js

+  fs.writeFileSync(cwd + '/package.json', JSON.stringify(packageJson));
+  t.deepEqual(getConfig(), sgcrc);
+  fs.unlinkSync(cwd + '/package.json');
+  fs.renameSync(cwd + '/package.json.back', cwd + '/package.json');


I totally forgot to change the directory for the linting. You might get some errors - can you change the directory tests in (package.json).scripts.lint to test?

from

"lint": "eslint lib tests",

to

"lint": "eslint lib test",

coveralls · 2017-03-15T08:49:36Z

Coverage decreased (-35.2%) to 50.0% when pulling 72fdf1a on feat/sgcrc into b97541b on master.

JPeer264 · 2017-03-15T09:00:52Z

Thanks @rudolfsonjunior

aichbauer added 2 commits March 14, 2017 18:59

🐛 Fix: remove ! from first if stmt

bb001e7

✨ Feat: added all types for .sgcrc_default

a7d3108

aichbauer requested a review from JPeer264 March 14, 2017 18:03

JPeer264 reviewed Mar 14, 2017

View reviewed changes

aichbauer added 8 commits March 14, 2017 21:31

👌 Code Review: fixed the test types

c656e2c

👌 Fix: add ! to if stmt

b9a6797

🔨 Refactor: delete Add: from types

8916c6a

Merge remote-tracking branch 'origin/master' into feat/sgcrc

2c99dec

🔨 Refactor: change sgc.types.prefix to sgc.types.type

5f94a45

🔧 Refactor: remove package.json .sgc

0603ca4

✅ Test: add test for .sgcrc_default, updated test for package.json .sgc

40ff350

🐛 Fix: updated stmt, pass with altPath, package.json and .sgcrc_default

7c08852

aichbauer requested a review from JPeer264 March 14, 2017 22:47

JPeer264 requested changes Mar 14, 2017

View reviewed changes

aichbauer added 2 commits March 15, 2017 00:14

🔧 Chore: add fs-extra

171abd8

✅ Test: update test for package.json, fs-extra

d6ee949

JPeer264 requested changes Mar 15, 2017

View reviewed changes

aichbauer added 4 commits March 15, 2017 09:40

🔧 Chore: change lint - tests to test

fc1cb38

🔨 Refactor: changed test promise to sync

b5c82f7

🔨 Refactor: skip test

b72135f

🔨 Refactor: changed path.join

72fdf1a

JPeer264 merged this pull request into master Mar 15, 2017

aichbauer deleted the feat/sgcrc branch March 15, 2017 11:41

Uh oh!

Conversation

aichbauer commented Mar 14, 2017

Uh oh!

coveralls commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aichbauer commented Mar 14, 2017

Uh oh!

JPeer264 left a comment

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!

aichbauer commented Mar 14, 2017

Uh oh!

JPeer264 commented Mar 14, 2017

Uh oh!

coveralls commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

coveralls commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JPeer264 commented Mar 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Mar 14, 2017 •

edited

Loading

coveralls commented Mar 14, 2017 •

edited

Loading

coveralls commented Mar 15, 2017 •

edited

Loading