Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop dependency on gulp-util #137

Merged
merged 1 commit into from
Dec 31, 2017

Conversation

ohbarye
Copy link
Contributor

@ohbarye ohbarye commented Dec 30, 2017

Hello maintainers.

Following gulpjs/gulp-util#143, I dropped dependency on gulp-util.

closes #136

Thanks!

@w0rm
Copy link
Member

w0rm commented Dec 30, 2017

@ohbarye hi! Looks good to me. What should I do with the version bump? Are these changes backwards compatible and work with the old gulp?

Copy link

@demurgos demurgos left a comment

Choose a reason for hiding this comment

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

Looks fine overall but vinyl should be in the devDependencies.

I also see that you used fancyLog.info instead of fancyLog. This does a console.info instead of console.log. I am not sure if this change has a real impact but I see why you did it: it's because the tests rely on the global cache of the modules to stub logging. It's not very clean but fixing it is a lot more involved: I am not even sure what is the best solution. Before I saw your PR, I wrote my own where I used fancyLog and exposed it as an internal property of postcss for testing. You can see it there.

@@ -50,6 +51,7 @@
"mocha": "^3.4.2",
"nyc": "^11.0.3",
"proxyquire": "^1.8.0",
"sinon": "^2.3.5"
"sinon": "^2.3.5",
"vinyl": "^2.1.0"

Choose a reason for hiding this comment

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

vinyl should be in the devDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, but vinyl is already in the devDependencies. 😄

https://github.com/ohbarye/gulp-postcss/blob/6f15b5127f66ba7c360625c1b5e2fec4f51c575e/package.json#L47-L56

Choose a reason for hiding this comment

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

You are right, I got confused.

@demurgos
Copy link

@w0rm
This should be a patch update from a semver point of view.

@w0rm
Copy link
Member

w0rm commented Dec 30, 2017

@demurgos thanks for taking a look!

@w0rm w0rm merged commit 0af46f5 into postcss:master Dec 31, 2017
@w0rm
Copy link
Member

w0rm commented Dec 31, 2017

@ohbarye thanks for your contribution, I've just released v7.0.1

@ohbarye ohbarye deleted the drop-dependency-on-gulp-util branch December 31, 2017 13:42
@ohbarye
Copy link
Contributor Author

ohbarye commented Dec 31, 2017

@w0rm Thanks for your review and merge!

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.

Replace deprecated dependency gulp-util
3 participants