ESLint pre-commit hook fails if there are problems #475

ghost · 2017-03-10T13:17:05Z

A user can choose to ignore this behavior by specifying bindly_pass option:

PreCommit:
  EsLint:
    enabled: true
    on_warn: fail
    bindly_pass: true

sds

Hey @domcorvasce, thanks for the pull request!

Can you help me understand why this is necessary? Also, did you mean for the name to be blindly_pass? (I'm not familiar with what bindly refers to in this context)

In general, this has me slightly skeptical due to the naming. I would like some concrete examples so we can come up with a better name.

Or even better: it seems like it would make more sense for the hook to warn when there is a problem parsing a file with eslint, rather than having the user explicitly specify an option to override this behavior (but since I don't write JavaScript I perhaps I don't understand the situations where this comes up).

I'm also concerned with the multiple typos in this PR. It suggests you haven't explicitly tested it yourself, which has me concerned about merging.

If you can address those issues I think we'll be in a much better place to merge here. Thanks!

sds · 2017-03-10T23:55:24Z

lib/overcommit/hook/pre_commit/es_lint.rb

@@ -12,19 +12,30 @@ module Overcommit::Hook::PreCommit
  #     enabled: true
  #     command: ['npm', 'run', 'lint', '--', '-f', 'compact']
  #
+  # If ESLint returns some errors, you can bindly pass by
+  # setting `bindly_pass` option to true.


Can we elaborate on the conditions where this is useful? (concrete examples are very helpful here)

Honestly, I don't think it can be useful.
But I thought that some people might want to pass ahead if something goes wrong with ESLint (I won't do it).

In that case, I think it's better to remove it completely. If not having the configuration option turns out to be an issue for people, we can always add it at that point.

sds · 2017-03-10T23:57:48Z

lib/overcommit/hook/pre_commit/es_lint.rb

+
+      # Fail for issues relative to the tool used.
+      if !config['bindly_pass'] && messages.empty? && !result.success?
+        $stderr.puts result.stderrs


Using $stderr.puts will break the formatting of the hook run. Output should be returned along with the return line below, e.g.:

return [:fail, result.stderr]

Also, stderrs is a typo (should be stderr).

I am very sorry for this.
I returned the stderrs instance variable.

sds · 2017-03-10T23:58:36Z

spec/overcommit/hook/pre_commit/es_lint_spec.rb

+    let(:result) { double('result') }
+
+    before do
+      result.stub(:stderrs).and_return('SyntaxError: Use of const in strict mode.')


stderrs -> stderr

ghost · 2017-03-11T06:32:47Z

Hello @sds!

First, thanks for the time you dedicate to this project and to me.
For the testing issue: I tested the changes locally and it works correctly, I tried multiple times before to open a pull request.

I am sorry 'cause I've not studied the code enough to avoid the typo I did.
I am going to fix everything during the day.

trotzig

If you remove the configuration option, I think this is good to go. However, it would still be useful to understand when this can happen. Perhaps you can provide some javascript file content and the result of running eslint on it? That could help better understand your change.

trotzig · 2017-03-11T17:17:25Z

lib/overcommit/hook/pre_commit/es_lint.rb

@@ -12,19 +12,30 @@ module Overcommit::Hook::PreCommit
  #     enabled: true
  #     command: ['npm', 'run', 'lint', '--', '-f', 'compact']
  #
+  # If ESLint returns some errors, you can bindly pass by
+  # setting `bindly_pass` option to true.


In that case, I think it's better to remove it completely. If not having the configuration option turns out to be an issue for people, we can always add it at that point.

trotzig · 2017-03-13T10:03:24Z

Thanks for following up @domcorvasce! The build failures weren't introduced by you so I went ahead and merged your changes.

Dom Corvasce added 2 commits March 10, 2017 14:04

Fix #458: Overcommit fails if ESLint fails for whatever reason

72cbcfa

Add check for bindly_pass option

68cb819

ghost changed the title ~~Fix issue #458 so ESLint fail if there are some problems~~ ESLint pre-commit hook fails if there are problems with ESLint Mar 10, 2017

ghost changed the title ~~ESLint pre-commit hook fails if there are problems with ESLint~~ ESLint pre-commit hook fails if there are problems Mar 10, 2017

Delete extra empty line

b21fcac

sds requested changes Mar 11, 2017

View reviewed changes

Return the stderr output along with exit status

59f2188

trotzig suggested changes Mar 11, 2017

View reviewed changes

Remove bindly_pass option

1063332

trotzig merged commit f8cd206 into sds:master Mar 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESLint pre-commit hook fails if there are problems #475

ESLint pre-commit hook fails if there are problems #475

ghost commented Mar 10, 2017

sds left a comment

sds Mar 10, 2017

ghost Mar 11, 2017

trotzig Mar 11, 2017

sds Mar 10, 2017

ghost Mar 11, 2017

sds Mar 10, 2017

ghost commented Mar 11, 2017 •

edited by ghost

Loading

trotzig left a comment

trotzig Mar 11, 2017

trotzig commented Mar 13, 2017

ESLint pre-commit hook fails if there are problems #475

ESLint pre-commit hook fails if there are problems #475

Conversation

ghost commented Mar 10, 2017

sds left a comment

Choose a reason for hiding this comment

sds Mar 10, 2017

Choose a reason for hiding this comment

ghost Mar 11, 2017

Choose a reason for hiding this comment

trotzig Mar 11, 2017

Choose a reason for hiding this comment

sds Mar 10, 2017

Choose a reason for hiding this comment

ghost Mar 11, 2017

Choose a reason for hiding this comment

sds Mar 10, 2017

Choose a reason for hiding this comment

ghost commented Mar 11, 2017 • edited by ghost Loading

trotzig left a comment

Choose a reason for hiding this comment

trotzig Mar 11, 2017

Choose a reason for hiding this comment

trotzig commented Mar 13, 2017

ghost commented Mar 11, 2017 •

edited by ghost

Loading