Skip to content

Added coffeelint support#25

Merged
kimroen merged 9 commits intokimroen:masterfrom
Myztiq:coffeelint
Nov 6, 2014
Merged

Added coffeelint support#25
kimroen merged 9 commits intokimroen:masterfrom
Myztiq:coffeelint

Conversation

@Myztiq
Copy link
Contributor

@Myztiq Myztiq commented Nov 4, 2014

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.

Copy link
Owner

Choose a reason for hiding this comment

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

The broccoli-filter-dependency is missing here.

@kimroen
Copy link
Owner

kimroen commented Nov 5, 2014

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

Thanks again 👍

@kimroen kimroen added this to the v0.3.0 milestone Nov 5, 2014
@Myztiq
Copy link
Contributor Author

Myztiq commented Nov 5, 2014

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
Copy link
Contributor Author

Myztiq commented Nov 5, 2014

Tested. Working. 👍

@kimroen
Copy link
Owner

kimroen commented Nov 5, 2014

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
Copy link
Contributor Author

Myztiq commented Nov 5, 2014

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
Copy link
Owner

kimroen commented Nov 5, 2014

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
Copy link
Contributor Author

Myztiq commented Nov 5, 2014

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
Copy link
Owner

kimroen commented Nov 5, 2014

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
Copy link
Contributor Author

Myztiq commented Nov 5, 2014

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
Copy link
Owner

kimroen commented Nov 5, 2014

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! 👍

kimroen added a commit that referenced this pull request Nov 6, 2014
@kimroen kimroen merged commit c65a9d6 into kimroen:master Nov 6, 2014
@Myztiq Myztiq deleted the coffeelint branch November 6, 2014 17:25
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