kimroen / ember-cli-coffeescript

Adds precompilation of CoffeeScript files and all the basic generation types to the ember generate command.
MIT License
72 stars 49 forks source link

Added coffeelint support #25

Closed Myztiq closed 10 years ago

Myztiq commented 10 years ago

Please let me know if there are any styling updates, or other such changes. I originally intended on using the broccoli-coffeelint library but it was triggering a bunch of errors so I used that as reference and rolled my own.

kimroen commented 10 years ago

Left some comments on styling and a missing dependency, but the actual implementation looks very nice.

Thanks again :+1:

Myztiq commented 10 years ago

I updated with the changes you specified, and I just added the ability to specify a custom formatter. The machine I am on right now does not have a good testing project setup, so the custom formatting support is untested right now. I can test it in about 2 hours, let me know if you beat me to the punch.

Myztiq commented 10 years ago

Tested. Working. :+1:

kimroen commented 10 years ago

Unfortunately, I think something is up with the broccoli-plugin setup here. When you run ember build, the filter runs twice, one time with all files, and then again with a fewer number of files (apparently not the same ones, as they have no errors). This might be due to something I did to test. Could you check for the same behavior on your end, just to make sure?

Second, we'll need to rename the broccoli-plugin to something other than CoffeeScriptFilter - that's the same name that broccolli-coffeescript is using, so it'll cause confusion. CoffeeScriptLinter?

Livereload server on port 35729
Serving on http://0.0.0.0:4200/
Running CoffeeScriptFilter#write to /Users/kimroen/Utvikling/PAM/webclient/tmp/coffee_script_filter-tmp_dest_dir-9ov39RBX.tmp

==========================================
File: webclient/authenticators/refresh.coffee
==========================================
 Unnecessary fat arrow
Line:  19
Level: warn

[...]

==========================================
File: webclient/views/radio-buttons.coffee
==========================================
 Line contains inconsistent indentation
Line:  19
Line:            content.find (obj) ->
Level: error

 Line contains inconsistent indentation
Line:  22
Line:            null
Level: error

=======================================================
Linting completed on 160 files with  10  errors.
=======================================================

Running CoffeeScriptFilter#write to /Users/kimroen/Utvikling/PAM/webclient/tmp/coffee_script_filter-tmp_dest_dir-XLCG8QaB.tmp

=======================================================
Linting completed on 65 files with  0  errors.
=======================================================

Build successful - 3893ms.

Slowest Trees                  | Total
-------------------------------+----------------
CoffeeScriptFilter             | 909ms
CoffeeScriptFilter             | 623ms
AutoprefixerFilter             | 566ms
ES6Concatenator                | 537ms
SassCompiler                   | 266ms
Myztiq commented 10 years ago

It runs the filter on the app and test trees, I am not sure if there is a tree we can run it on that contains both.

CoffeeScriptFilter changed to CoffeeScriptLinter.

kimroen commented 10 years ago

Ah, right. Of course it does. I'd like to make it clearer that that's happening.

Also, the linting is taking longer than the CoffeeScript compilation right now - I wonder if something can be done about that. It seems like broccoli-filter is copying files, I'm not sure if that's necessary.

Myztiq commented 10 years ago

I don't know if there are any other methods to watch for individual file changes without using broccoli-filter like this.

I am not sure if there is a good way to determine if we are running on the testing or app folders. We might be able to inspect the path of the first file we get sent and see if it matches a pattern. The problem there is the path does not mean anything because the tree can be built dynamically.

kimroen commented 10 years ago

I thought I remembered a way to check it, but I was actually thinking of postprocessTree, which I used here, in ember-cli-autoprefixer: https://github.com/kimroen/ember-cli-autoprefixer/blob/master/index.js#L13

There might be something to learn from the broccoli-jshint-implementation though - as far as I can see broccolli-coffeelint was based on that: https://github.com/rwjblue/broccoli-jshint/blob/master/index.js#L29-L48

Myztiq commented 10 years ago

broccoli-jshint is using Filter.prototype.write.apply which is in fact copying the files. In our case it's even working with symlinks so that should not be a speed issue. I think it's actually the linting process that might be taking a long time.

kimroen commented 10 years ago

I think this is worth merging in either way though. I'll look over it one more time tomorrow, and then merge if I don't see any showstoppers.

Thanks again! :+1: