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

Allow gulp command to check which jigsaw binary to use #85

Merged
merged 3 commits into from
Mar 28, 2017
Merged

Allow gulp command to check which jigsaw binary to use #85

merged 3 commits into from
Mar 28, 2017

Conversation

erickpatrick
Copy link
Contributor

When running gulp, instead of making the user change the gulp file, manually,
add a check for which jigsaw binary to use (global or local under vendor
folder) and in the case none exists prints a message to stdout via
console.log and exits the process.

When running gulp, instead of making the user change the gulp file, manually,
add a check for which jigsaw binary to use (global or local under vendor
folder) and in the case none exists prints a message to stdout via
console.log and exits the process.
@damiani
Copy link
Contributor

damiani commented Mar 23, 2017

Fantastic! Could you add a little bit to the console error message, to mention using Composer? Something like Could not find Jigsaw; please install it via Composer, either locally or globally.

👍

@erickpatrick
Copy link
Contributor Author

Sure, I'll work on that =)

Instead of polluting gulpfile with extra code, move the path
discovery to its own custom local node module which will
be required in the gulpfile.

The reasoning behind this was to have a more robust path locator.
@erickpatrick
Copy link
Contributor Author

Hi @damiani, please, check the changes I made. After some testing I decide to change how I approached this, but now it seems to be working better than the previous one. See the commit message.

Also, I added the message you asked.

@damiani
Copy link
Contributor

damiani commented Mar 28, 2017

Looks great, though I believe we don't need the added fs dependency in package.json, since fs is part of the core.

@erickpatrick
Copy link
Contributor Author

Hi @damiani, removed the dependency. If you think it can be merged, let me know if you rather have it squashed into a single commit or if you're going to do that by GitHub interface.

@damiani
Copy link
Contributor

damiani commented Mar 28, 2017

Excellent, thanks for this!

@damiani damiani merged commit 9e44f42 into tighten:master Mar 28, 2017
@erickpatrick
Copy link
Contributor Author

@damiani my pleasure! Expect more to come, I'm really enjoying working with jigsaw

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.

None yet

2 participants