Skip to content

Conversation

@sttk
Copy link
Contributor

@sttk sttk commented Jun 30, 2016

This PR is for adapting to both v1.0.0 and previous versions of undertaker.
(And this adds a test case for 100% coverage.)

install:
- IF %nodejs_version% EQU 0.10 npm -g install npm@2
- IF %nodejs_version% EQU 0.10 set PATH=%APPDATA%\npm;%PATH%
- npm -g install npm@latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure you want to be using npm 3? It's so much slower, and appveyor is already extremely slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm install EPERM errors frequently occured on appveyor, and it seems that this error can be fixed by using npm3.

ref. npm/npm#9696

@sttk sttk force-pushed the adapt_to_undertaker_v1 branch from ce579d9 to e526551 Compare July 2, 2016 03:00
@sttk sttk force-pushed the adapt_to_undertaker_v1 branch from e526551 to 7d7a4e8 Compare July 2, 2016 05:04
@sttk
Copy link
Contributor Author

sttk commented Jul 2, 2016

I've finished modifying the codes.
I used lodash to check the properties and modified the same check in tasks.js, too.
In addition, I've added node 6 to the test target.

function isObject(val) {
return typeof val === 'object' && !Array.isArray(val);
}
var _ = require('lodash');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think people were trying to transition all the gulp dependencies to use lodash.* packages, so this should require lodash.sortby, lodash.isstring and lodash.isplainobject

@phated
Copy link
Member

phated commented Jul 11, 2016

@sttk sorry it took me so long to review. I've commented on some things I noticed.

@sttk
Copy link
Contributor Author

sttk commented Jul 12, 2016

@phated Don't worry. I know you are busy with other projects. Thank you for reviewing.

@sttk
Copy link
Contributor Author

sttk commented Jul 15, 2016

@phated I've modified what you pointed out. And I've merged the tests for the flag --tasks in one.

@phated phated merged commit 0cd8050 into gulpjs:master Jul 15, 2016
@phated
Copy link
Member

phated commented Jul 15, 2016

Published as 1.2.2

@phated
Copy link
Member

phated commented Jul 15, 2016

Excellent work @sttk! Really glad you caught this oversight in my undertaker changes.

@sttk sttk deleted the adapt_to_undertaker_v1 branch July 16, 2016 13:11
phated pushed a commit that referenced this pull request Dec 21, 2017
phated pushed a commit that referenced this pull request Dec 21, 2017
phated pushed a commit that referenced this pull request Dec 21, 2017
phated pushed a commit that referenced this pull request Dec 21, 2017
phated pushed a commit that referenced this pull request Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants