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

Bug Fix: Issue#4: The gulp process should not exit now #373

Closed
wants to merge 1 commit into from

Conversation

krshubham
Copy link
Contributor

Issue 4

The basic idea was to use an error handler with each pipe and emit 'end' event to the corresponding pipe.

@atfornes
Copy link
Contributor

Thanks @krshubham, could you please review the indentation and trailing spaces using our jshint rules in your editor?

Using Atom editor is really easy to setup the editor to have automatic indentation.

@krshubham
Copy link
Contributor Author

@atfornes , I have set the indentation to 2 spaces. I hope that's fine as I see the same value written in the .jshintrc file

Copy link
Contributor

@atfornes atfornes left a comment

Choose a reason for hiding this comment

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

Please, review indentation. We use https://github.com/Grasia/teem/blob/master/.jshintrc config file and jshint tool to ensure this. Please include it in your development settings for future pull requests

gulpfile.js Outdated
/*================================================
= Report Errors to Console =
================================================*/

gulp.on('error', function(e) {
gulp.on('error', function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please, erase trailing white spaces. Using a good linter that uses our jshint rules is the preferred way of doing it.

}

return stream.pipe(gulp.dest(path.join(config.dest, 'images')));
return stream.pipe(gulp.dest(path.join(config.dest, 'images')))
.on('error', endErrorProcess);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this .on should be indented two spaces towards the right. Please review that it follows our jshint rules.

@@ -530,7 +579,9 @@ function buildManifest (env) {
exclude: 'app.manifest',
hash: true
}))
.pipe(gulp.dest(config.dest));
.on('error', endErrorProcess)
.pipe(gulp.dest(config.dest))
Copy link
Contributor

Choose a reason for hiding this comment

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

please review indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this, I'll correct this and keep in mind from next time. Also, I'll for sure configure my text editor or use atom as you said its easy to get it configured. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

don't worry :) just sharing our formatting tools and rules

@atfornes
Copy link
Contributor

@krshubham, can you please open another pull request for the popover?

In addition, please check that:

  1. you have jslint and jshint enabled in your editor and that your new code comply with our rules
  2. Please do not add in your commit indentation changes in lines that you have not modified.

Thanks!

@krshubham
Copy link
Contributor Author

@atfornes I am closing this right now and will again make a PR as I messed up in this! 😢

@krshubham krshubham closed this Jun 22, 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