chore: remove gulp-git dependency #256

marionebl · 2016-06-11T23:31:28Z

gets rid of gulp dependency
gets rid of gulp-git dependency
solves gulp-related deprecation warnings
improves install times considerably
use child_process.exec for git commands

fix coverage
fix windows support
restore cwd switching to commit and log

marionebl · 2016-06-16T07:56:00Z

Hey there, this should be done now. Tell me what you think – I'd squash this if you are fine with the idea

LinusU · 2016-06-16T08:10:00Z

src/cli/strategies/git-cz.js

- });
-
+ let cleanError = new Error('Error: No files added to staging! Did you forget to run git add?');
+ throw cleanError;


Why assign the error to a variable first?

marionebl · 2016-06-18T10:30:10Z

@LinusU I removed all the ignore comments and refactored git/commit to a way more concise format, causing the tests to fulfill the coverage thresholds.

LinusU · 2016-06-19T08:24:45Z

src/cli/strategies/git-cz.js

- // console.log('commit happened');
- });
-
+ throw new Error('Error: No files added to staging! Did you forget to run git add?');


You can remove the Error: part from the start of the message, the error class will add that when casting to a string.

LinusU · 2016-06-19T08:27:08Z

Can you rebase this on latest master now that your two other pull requests got merged? Remember to remove those commit when rebasing so that they don't get added twice...

marionebl · 2016-06-19T09:27:40Z

Rebased master and fixed up all commits, reworded base commit from "chore" to "refactor" type.

marionebl · 2016-06-21T14:20:08Z

Ping!

marionebl · 2016-06-24T17:51:46Z

Any news on this?

LinusU · 2016-06-27T14:13:29Z

Sorry, I've been away celebrating Swedish midsummer 🇸🇪 🌳 🍸

I'll try and look at this shortly

LinusU · 2016-06-28T19:23:35Z

It doesn't seem like you are removing gulp and gulp-git from the package.json, could you do that?

LinusU · 2016-06-28T19:25:11Z

Instead of using child_process.exec, would it be better to use execFile or spawn so that we can give the arguments as an array instead as a string. That way we don't have to worry about quoting at all which I think would improve that part of the code base.

* gets rid of gulp dependency * gets rid of gulp-git dependency * solves gulp-related deprecation warnings * improves install times considerably * use child_process.exec for git commands

marionebl · 2016-06-28T22:47:41Z

Removed gulp and gulp-git from .dependencies. I'd stay with child_process's exec for now because git/commit.js needs

normalizeCommitMessage for quote normalization anyway
would need to parse options.args somehow, which currently allows injection of arbitrary args
takes on some complexity for error handling when changing to spawn

LinusU · 2016-06-29T05:50:03Z

src/commitizen/staging.js

- stagingIsClean = false;
- } else {
- stagingIsClean = true;
+ exec('git diff --cached --name-only', {


Should we keep --no-pager here?

I think --no-pager isn't needed here, as the succeeding tests at test/tests/staging show. I guess the option was intended to guard against counting the length of the pager (END) string. According to my tests git diff output is not paged anyway in non-tty processes.

LinusU · 2016-06-29T05:52:42Z

I'd stay with child_process's exec for now because...

Fair enough 👌

One last question but otherwise I'm okay on landing this.

@jimthedev any feelings on this?

jimthedev · 2016-06-29T09:10:55Z

I've been listening and at this point I'm ok with landing this. You've
covered all of the major topics. One thing we might want to do is check the
old vs new output to stdout to make sure that any changes in that regard
are documented. It is tricky to tell how other projects might be using that
output, but if it is a significant difference then we may want to release
it as a major version update.

Otherwise I'm totally on board with removing these.

On June 29, 2016 at 12:52:42 AM, Linus Unnebäck ([email protected])
wrote:

I'd stay with child_process's exec for now because...

Fair enough 👌

One last question but otherwise I'm okay on landing this.

@jimthedev https://github.com/jimthedev any feelings on this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#256 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAGpitqXEaUe1cKYMAJhqzEn-5qFPoIcks5qQggqgaJpZM4Izqgw
.

marionebl · 2016-06-29T11:55:41Z

Concerning stdout, stderr and exit codes:

Changes

cli/strategies/git-cz.js Staging check now exits with code 1 if the stage is clean. stderr output now contains a stack trace below the error message.
cli/strategies/git-cz.js Committing now exits with code 1 if committing fails instead of failing silently. stderr output now contains a stack trace.
cli/strategies/git.js Committing now exits with code 1 if committing fails instead of just printing an error message. stderr output now contains a stack trace below the error message.

Important unchanged stuff

git/commit.js still respects the quiet option. Output should be exactly the same as with gulp-git in success scenarios. When committing errors the git-cz strategy kicks in and the changes there apply.

So there are changes that could break stuff for people relying on

git-cz currently alway existing with code 0
git-cz printing specific strings in error cases as last output lines on stderr

Given the fact there was no other way for cli consumers to tell success from error except checking for output on stderr point 2 seems likely enough to warrant a major version bump.

I could add a Breaking Change notice to the commit body if you are on the same page.

marionebl · 2016-07-01T23:11:19Z

Major version bump for these changes?

marionebl · 2016-07-06T09:06:52Z

Any news?

LinusU · 2016-07-20T21:59:46Z

Sorry again for delays. I would prefer not to make a major bump since I don't see that this would have an impact on current users...

I have a really hard time imagining that changing the error output would be a problem, unless someone is doing exact string comparison on errors which seems unlikely.

Someone could potentially be relaying on the exit code always being 0. However, I don't think that many users, if any at all, does this since it's very very non-standard to always exit with a success code. If I where do write a script that expected any failures I would first of all assume that this utility actually exited with an error code, and I would have opened an issue when I found out that it didn't.

I'm okay with merging this now and releasing as a patch release. @jimthedev should have final say though, if you just give the go ahead I will happily merge and cut a new release 👌

marionebl · 2016-08-01T21:02:54Z

Hey @LinusU @jimthedev, can we go forward on this?

jimthedev · 2016-08-01T21:24:29Z

I am on board with @LinusU's plan.

LinusU · 2016-08-02T06:08:34Z

Great, thank you for your work @marionebl!

LinusU reviewed Jun 16, 2016
View reviewed changes

marionebl force-pushed the chore/remove-gulp-git-dependency branch from c89c06d to 54e5c15 Compare June 18, 2016 10:06

marionebl mentioned this pull request Jun 18, 2016

chore: save npm dependencies with pinned version by default #261

Merged

LinusU reviewed Jun 19, 2016
View reviewed changes

marionebl force-pushed the chore/remove-gulp-git-dependency branch from 54e5c15 to d8afcfb Compare June 19, 2016 09:16

marionebl force-pushed the chore/remove-gulp-git-dependency branch from d8afcfb to 955eb42 Compare June 28, 2016 22:37

refactor: remove gulp-git dependency

dbf16d8

* gets rid of gulp dependency * gets rid of gulp-git dependency * solves gulp-related deprecation warnings * improves install times considerably * use child_process.exec for git commands

marionebl force-pushed the chore/remove-gulp-git-dependency branch from 955eb42 to dbf16d8 Compare June 28, 2016 22:40

LinusU reviewed Jun 29, 2016
View reviewed changes

LinusU mentioned this pull request Jul 20, 2016

fix(commit): Adds support for multilines on Windows #287

Closed

LinusU merged commit 0d4e0d5 into commitizen:master Aug 2, 2016

marionebl deleted the chore/remove-gulp-git-dependency branch August 2, 2016 06:21

marionebl mentioned this pull request Aug 2, 2016

git-cz doesn't return an error code when commit fails or is canceled #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove gulp-git dependency #256

chore: remove gulp-git dependency #256

marionebl commented Jun 11, 2016 •

edited

Loading

marionebl commented Jun 16, 2016 •

edited

Loading

LinusU Jun 16, 2016

marionebl Jun 16, 2016

marionebl commented Jun 18, 2016

LinusU Jun 19, 2016

LinusU commented Jun 19, 2016

marionebl commented Jun 19, 2016

marionebl commented Jun 21, 2016

marionebl commented Jun 24, 2016

LinusU commented Jun 27, 2016

LinusU commented Jun 28, 2016

LinusU commented Jun 28, 2016

marionebl commented Jun 28, 2016

LinusU Jun 29, 2016

marionebl Jun 29, 2016

LinusU commented Jun 29, 2016

jimthedev commented Jun 29, 2016

marionebl commented Jun 29, 2016 •

edited

Loading

marionebl commented Jul 1, 2016

marionebl commented Jul 6, 2016

LinusU commented Jul 20, 2016

marionebl commented Aug 1, 2016

jimthedev commented Aug 1, 2016

LinusU commented Aug 2, 2016

chore: remove gulp-git dependency #256

chore: remove gulp-git dependency #256

Conversation

marionebl commented Jun 11, 2016 • edited Loading

marionebl commented Jun 16, 2016 • edited Loading

LinusU Jun 16, 2016

Choose a reason for hiding this comment

marionebl Jun 16, 2016

Choose a reason for hiding this comment

marionebl commented Jun 18, 2016

LinusU Jun 19, 2016

Choose a reason for hiding this comment

LinusU commented Jun 19, 2016

marionebl commented Jun 19, 2016

marionebl commented Jun 21, 2016

marionebl commented Jun 24, 2016

LinusU commented Jun 27, 2016

LinusU commented Jun 28, 2016

LinusU commented Jun 28, 2016

marionebl commented Jun 28, 2016

LinusU Jun 29, 2016

Choose a reason for hiding this comment

marionebl Jun 29, 2016

Choose a reason for hiding this comment

LinusU commented Jun 29, 2016

jimthedev commented Jun 29, 2016

marionebl commented Jun 29, 2016 • edited Loading

marionebl commented Jul 1, 2016

marionebl commented Jul 6, 2016

LinusU commented Jul 20, 2016

marionebl commented Aug 1, 2016

jimthedev commented Aug 1, 2016

LinusU commented Aug 2, 2016

marionebl commented Jun 11, 2016 •

edited

Loading

marionebl commented Jun 16, 2016 •

edited

Loading

marionebl commented Jun 29, 2016 •

edited

Loading