search

clutchski / coffeelint

Lint your CoffeeScript.
http://www.coffeelint.org
Other
1.18k stars 171 forks source link

Several enhancements, including CJSX fixes #632

Closed aminland closed 4 years ago

aminland commented 6 years ago

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

aminland commented 6 years ago

Also fixes #630 and #619

aminland commented 6 years ago

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 commented 6 years ago

Hi. Thanks for the PR

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

  1. Update CoffeeScript, which I'll check and handle in a bit.
  2. Some kind of config extender? Not sure why that isn't mentioned at all in your description.
  3. 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 commented 6 years ago

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: 1) 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).

2) 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.

3) 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

bump.

UziTech commented 4 years ago

This will be added to @coffeelint/cli in https://github.com/coffeelint/coffeelint/pull/8. Thanks!