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

Fix static checking support (jshint, coffeelint, tslint) #279

Closed
wants to merge 1 commit into from

Conversation

Toilal
Copy link
Contributor

@Toilal Toilal commented Jan 21, 2015

This close #277, but also moves all jshint/coffeelint/tslint to a static.js gulp file, support two tasks : static and static:fail.

static only displays warning, but static:fail also raise an error.

Those tasks are registered in gulp build (static:fail) and gulp watch (static).

<% if (props.jsPreprocessor.extension === 'js') { %>
.pipe($.jshint())
.pipe($.jshint.reporter('jshint-stylish'))
<% } if (props.jsPreprocessor.key !== 'none') { %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove this line because it's dead code, as gulp/script.js won't be included if props.jsPreprocessor.key === 'none'.

@Toilal Toilal force-pushed the feat/static branch 4 times, most recently from 559062f to c2ff5f9 Compare January 23, 2015 20:45
@Swiip
Copy link
Owner

Swiip commented Jan 26, 2015

I don't like adding a new gulp script. We could use the scripts one to run jshint when no js preprocessor is selected.

@Toilal
Copy link
Contributor Author

Toilal commented Jan 26, 2015

So i remove https://github.com/Swiip/generator-gulp-angular/blob/master/app/src/format.js#L203-L205 for script to be included when no js preprocessor ?

@Toilal
Copy link
Contributor Author

Toilal commented Jan 26, 2015

And what about watch task ? We don't need to run linters on each changes ?

@Toilal
Copy link
Contributor Author

Toilal commented Jan 26, 2015

@Swiip please tell me your thinkings about the current implementation.

I've added a global gulp.context object containing variables used in scripts task. This global object is configured in watch task to support no failing on error. I think it's easier than having two distinct tasks like before (static and static:fail)

@Swiip
Copy link
Owner

Swiip commented Feb 14, 2015

Sorry for the looong delay. I like the evolutions except for the context variables. It could be fixed by merging with #335. Wait for the #335 to me merged, merge with master and we'll be good to go.

@Toilal
Copy link
Contributor Author

Toilal commented Feb 14, 2015

@Swiip No problem, just ping me when #335 is merged.

@Swiip
Copy link
Owner

Swiip commented Feb 16, 2015

Ping :)

@zckrs
Copy link
Collaborator

zckrs commented Feb 18, 2015

Ping @Toilal

@Toilal
Copy link
Contributor Author

Toilal commented Feb 19, 2015

Sorry for delay, i'm on it.

@Toilal
Copy link
Contributor Author

Toilal commented Feb 19, 2015

I've rebased locally, but npm test fails. It also fails locally using master.

@Toilal
Copy link
Contributor Author

Toilal commented Feb 19, 2015

gulp-util is missing.

@Toilal
Copy link
Contributor Author

Toilal commented Feb 19, 2015

Ok, i needed to remove old temporary directories from test directory. Test are OK now.

@Toilal Toilal force-pushed the feat/static branch 3 times, most recently from c4f4d49 to 9cbeff7 Compare February 19, 2015 08:29
@Toilal
Copy link
Contributor Author

Toilal commented Feb 19, 2015

Ready to merge I think. Note that gulp/script.js is always included now, because is contains linting even when no js pre-processor is chosen.

errorHandler: function(title) {
failOnLintErrors: true,
failOnCompileErrors: true,
errorHandler: function(title, throw_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why throw_ ? strange name for a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw is a reserved keyword
Le 20 févr. 2015 11:33, "Mehdy Dara" notifications@github.com a écrit :

In app/templates/_gulpfile.js
#279 (comment)
:

@@ -10,10 +10,16 @@ var options = {
dist: '<%= props.paths.dist %>',
tmp: '<%= props.paths.tmp %>',
e2e: '<%= props.paths.e2e %>',

  • errorHandler: function(title) {
  • failOnLintErrors: true,
  • failOnCompileErrors: true,
  • errorHandler: function(title, throw_) {

Why throw_ ? strange name for a variable


Reply to this email directly or view it on GitHub
https://github.com/Swiip/generator-gulp-angular/pull/279/files#r25061614
.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah throw is reserved. rename throw_ to error

@Toilal
Copy link
Contributor Author

Toilal commented Feb 20, 2015

Any idea why it fails on iojs ?

@zckrs
Copy link
Collaborator

zckrs commented Feb 20, 2015

Maybe conflict with node-sass
https://travis-ci.org/Swiip/generator-gulp-angular/jobs/51498522#L1222

I restart build

@zckrs
Copy link
Collaborator

zckrs commented Feb 20, 2015

sass/node-sass#532

@zckrs
Copy link
Collaborator

zckrs commented Feb 23, 2015

Can you rebase your branch with master ? We fix travis by ignore fail on iojs.

@Toilal Toilal force-pushed the feat/static branch 2 times, most recently from aa0783c to 1922a8b Compare February 27, 2015 08:13
@Toilal
Copy link
Contributor Author

Toilal commented Mar 3, 2015

I've just rebased this one, could you merge ?

@zckrs zckrs mentioned this pull request Mar 4, 2015
5 tasks
@Swiip Swiip closed this in #396 Mar 4, 2015
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.

jshint check is not performed
3 participants