Several enhancements, including CJSX fixes #632

aminland · 2018-01-16T23:55:50Z

Fixes colon and comma spacing issues from CSX (JSX) format described in #629

aminland · 2018-01-17T03:51:22Z

Also fixes #630 and #619

aminland · 2018-02-12T14:30:00Z

Until such time as you start accepting pull requests again, this code (with many more changes) will live at https://github.com/aminland/coffeelint2

named coffeelint2 for coffeescript 2 support. (also works nicely with eslint via my eslint-plugin-coffee)

swang · 2018-02-16T07:13:14Z

Hi. Thanks for the PR

There seems to be 3 separate requests in this one PR.

Update CoffeeScript, which I'll check and handle in a bit.
Some kind of config extender? Not sure why that isn't mentioned at all in your description.
I haven't found a proper solution to CSX but solving it should fix the issue for all rules. The code seems to only address 3 rules? Fixing coffeelint to handle CSX sounds very complicated without having some initial discussion first. To me handling CSX seems to entail something like:
a. having the base lintToken class ignore everything in-between CSX tags but still lint the attributes and interpolated code generated.
b. have a separate "rule" to handle CSX, including separate (from the rest of coffeelint) rules for spacing/linting attributes and interpolated code.
c. Similar to a but have some "parseCSX" property in the class of the rules that signifies whether the rules want to parse CSX or not.

I haven't implemented anything because I haven't spent much time with CSX/CS2. I think if someone started a discussion and had a good way to implement it, I would merge it. But I think handling it for each individual rule sounds like a nightmare.

Also it looks like you deleted some tests and some code that don't seem clear to me why you're deleting them. The tests added should also provide enough coverage where I feel confident that the implementation is sound. I'm not saying I need 100 tests, but the tests should appear to have coverage for the general cases.

aminland · 2018-02-23T06:15:48Z

Hi,

I've added more items since you last looked. In an effort to make this easier I rebased the hell out of my commits so that they're each somewhat self-contained in what they do.

In terms of the issues you raised:

the config extend commit is so that one can, let's say, in package.json coffeeLintConfig, extend a relative path (e.g. "extends": "../coffeeLintConfig/index"). Without this commit you can only extend things that are installed in node_modules (thus preventing you from dynamically generating a config locally).
Surprisingly, you don't need to do all that to get CJSX files to work correctly. The reason is simple: since we're using the CoffeeScript lexer, and that lexer supports CJSX via a hack of sorts, there's not much required to support it. At least in terms of the default rule configs, these basic changes are sufficient.
If you find other situations where CJSX tags break other rules, I'll be glad to fix those as well.

What this PR doesn't do is enable new rules to be written that specifically apply only in a CJSX context.

That last commit adds column numbers to the context for all errors (and for lexical lint rules, end positions as well). This is done so that when using an editor you can get proper highlighting.

--
The way I see it, coffeelint is mostly for syntax validation, and coffee-style preference checking. All of these changes were basically done with the aim of not running coffeelint directly, but rather, just using eslint. Eslint is much more powerful, and can do all sorts of extra things. With these changes in place, I was able to make eslint-plugin-coffee which enables full compatibility of eslint with coffee (and allowing direct checking / configuration of coffeelint style rules).

aminland · 2018-02-23T06:17:10Z

Cakefile

@@ -24,11 +24,17 @@ task 'compile:commandline', 'Compiles commandline.js', ->
    coffeeSync 'src/cache.coffee', 'lib/cache.js'
    coffeeSync 'src/ruleLoader.coffee', 'lib/ruleLoader.js'
    fs.mkdirSync 'lib/reporters' unless fs.existsSync 'lib/reporters'
+    fs.mkdirSync 'lib/rules' unless fs.existsSync 'lib/rules'


This is needed so we can export the default rules for use in eslint.

aminland · 2018-02-23T06:23:51Z

coffeelint.json

@@ -18,6 +18,9 @@
    "space_operators": {
        "level": "error"
    },
+    "max_line_length": {
+        "value": "100"


80 is just wayyy too low, it was causing issues in the test files. Easier to just up the limit than to adhere to a dogmatic 80.

aminland · 2018-02-23T06:24:49Z

package.json

@@ -32,8 +32,8 @@
    "strip-json-comments": "^1.0.2"
  },
  "devDependencies": {
-    "vows": ">=0.8.1",
-    "underscore": ">=1.4.4"
+    "underscore": ">=1.4.4",


npm automatically alphabeticalizes now.

aminland · 2018-02-23T06:26:05Z

src/coffeelint.coffee

@@ -310,9 +309,10 @@ coffeelint.lint = (source, userConfig = {}, literate = false) ->

    # only do further checks if the syntax is okay, otherwise they just fail
    # with syntax error exceptions
-    unless hasSyntaxError(source)
+    tokens = getTokens(source)


Here we were passing the source to Coffeescript before twice, which basically means we're doing twice as much work as needed.

aminland · 2018-02-23T06:27:11Z

src/configfinder.coffee

+            return null if not config[attr]?
+            config = config[attr]
+
+        config.__location__ = filename


This is so we can remember where the config file is loaded from, so we can resolve the "extends" property relative to the location of the configfile (as opposed to the cwd)

aminland · 2018-02-23T06:28:00Z

src/configfinder.coffee

    unless config.extends
        return config
+    try
+        parentConfig = require resolve config.extends,


if we cant resolve the path relative to our present directory, then we can look in node_modules

aminland · 2018-02-23T06:28:45Z

src/lexical_linter.coffee

@@ -18,10 +18,10 @@ BaseLinter = require './base_linter.coffee'
 #
 module.exports = class LexicalLinter extends BaseLinter

-    constructor: (source, config, rules, CoffeeScript) ->
+    constructor: (source, config, rules, CoffeeScript, tokens) ->


This is so we can pass in the tokens from coffeelint.coffee (so we're not running CoffeeScript.tokens(source) twice)

aminland · 2018-02-23T06:29:43Z

src/rules/colon_assignment_spacing.coffee

@@ -57,7 +60,8 @@ module.exports = class ColonAssignmentSpacing
        [isLeftSpaced, leftSpacing] = checkSpacing 'left'
        [isRightSpaced, rightSpacing] = checkSpacing 'right'

-        if isLeftSpaced and isRightSpaced
+        if token.csxColon or isLeftSpaced and isRightSpaced


we have no control over spacing of "csxColons" (these are autogenerated by the compiler)

aminland · 2018-02-23T06:34:31Z

src/rules/no_nested_string_interpolation.coffee

-            @trackStringStart()
+        @blocks = []
+
+    lintToken: (token, tokenApi) ->


I rewrote this function from scratch. To test for nested interpolation the old way is problematic since {} code blocks inside CSX tags are treated as nested interpolation in the old scheme.
In the new way, we keep track of visited string_start and string_end per csx tag. Each time we encounter a csx tag, we nest to another 'block', with its own tracking.

aminland · 2018-02-23T06:36:26Z

src/rules/prefer_english_operator.coffee

@@ -5,6 +5,7 @@ module.exports = class PreferEnglishOperator
        level: 'ignore'
        message: 'Don\'t use &&, ||, ==, !=, or !'
        doubleNotLevel: 'ignore'
+        ops: ['and', 'or', 'not', 'is', 'isnt']


This was just a preference thing. Rather than making a new custom rule that does a subset of what this rule does, the rule is now configurable so people can specify which english operators they prefer. Much nicer this way, for those who aren't a fan of "isnt" as an operator...

aminland · 2018-02-23T06:37:26Z

src/rules/spacing_after_comma.coffee

+    # Coffeescript does some code generation when using CSX syntax, and it adds
+    # brackets & commas that are not marked as generated. The only way to check
+    # these is to see if the comma has the same column number as the last token.
+    isGenerated: (token, tokenApi) ->


This function can be simplified later if the underlying coffeescript lexer starts marking things as generated.

…ility

…, not just in node_modules

…put)

aminland · 2018-02-23T06:58:59Z

If you really want me to split to multiple PRs I can, but i don't see a need as I don't think i've done anything controversial...

jaredmoody · 2018-04-07T00:01:18Z

What's the status here, can this be merged - or should separate PRs be submitted - or something else?

I need jsx support on my project so would be nice to have this on the main repo instead of having to use a different package.

aminland · 2018-08-23T14:51:12Z

bump.

UziTech · 2020-01-14T07:06:49Z

This will be added to @coffeelint/cli in coffeelint/coffeelint#8. Thanks!

aminland force-pushed the patch-1 branch 3 times, most recently from e4e882e to 7e80ca0 Compare January 17, 2018 03:48

aminland force-pushed the patch-1 branch 5 times, most recently from c590760 to c673321 Compare February 6, 2018 08:46

aminland changed the title ~~Don't break on CSX property assignment~~ Support for Coffeescript 2 (including JSX) Feb 14, 2018

aminland force-pushed the patch-1 branch from c673321 to 72d83d3 Compare February 23, 2018 05:49

aminland commented Feb 23, 2018

View reviewed changes

aminland force-pushed the patch-1 branch from 72d83d3 to f9be873 Compare February 23, 2018 06:22

aminland commented Feb 23, 2018

View reviewed changes

aminland added 4 commits February 23, 2018 01:42

Fix colon_assignment_spacing and spacing_after_comma for CSX compatib…

019bd2b

…ility

Fix nested string interpolation checks in CSX

5d5da21

Allow specification of ops for prefer_english_operator

9c65f29

Look up the "extends" property correctly based on path or config file…

65b3b83

…, not just in node_modules

aminland added 3 commits February 23, 2018 01:42

loosen the max line length

e10b4e0

Speedup by caching parsed tokens when checking for syntax error

0f32767

Compatibility for eslint plugin, (mainly adding column numbers in out…

e370247

…put)

aminland force-pushed the patch-1 branch from f9be873 to e370247 Compare February 23, 2018 06:42

aminland changed the title ~~Support for Coffeescript 2 (including JSX)~~ Several enhancements, including CJSX fixes Feb 23, 2018

fmasuhr mentioned this pull request Mar 31, 2018

coffeelint.json configuration files in SublimeLinter 4 SublimeLinter/SublimeLinter-coffeelint#29

Closed

UziTech mentioned this pull request Jan 14, 2020

Several enhancements, including CJSX fixes coffeelint/coffeelint#8

Merged

aminland closed this Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several enhancements, including CJSX fixes #632

Several enhancements, including CJSX fixes #632

aminland commented Jan 16, 2018 •

edited

Loading

aminland commented Jan 17, 2018

aminland commented Feb 12, 2018

swang commented Feb 16, 2018

aminland commented Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018

aminland Feb 23, 2018 •

edited

Loading

aminland commented Feb 23, 2018

jaredmoody commented Apr 7, 2018

aminland commented Aug 23, 2018

UziTech commented Jan 14, 2020

Several enhancements, including CJSX fixes #632

Several enhancements, including CJSX fixes #632

Conversation

aminland commented Jan 16, 2018 • edited Loading

aminland commented Jan 17, 2018

aminland commented Feb 12, 2018

swang commented Feb 16, 2018

aminland commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminland Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

aminland commented Feb 23, 2018

jaredmoody commented Apr 7, 2018

aminland commented Aug 23, 2018

UziTech commented Jan 14, 2020

aminland commented Jan 16, 2018 •

edited

Loading

aminland Feb 23, 2018 •

edited

Loading