schorfES / node-lintspaces

A validator for checking different kinds of whitespaces in your files.
https://npmjs.com/package/lintspaces
MIT License
30 stars 17 forks source link

.editorconfig option from dependency not working, even though file exists #30

Closed localjo closed 8 years ago

localjo commented 8 years ago

I've been having trouble with the editorconfig option (see https://github.com/AlbertoElias/gulp-lintspaces/issues/4).

When I pass in editorconfig: '.editorconfig' the feature works as expected. However, if I try to pass in an .editorconfig file from one of my dependencies (editorconfig: './node_modules/@myorg/code-standards/.editorconfig'), I don't get any linting errors reported. I verified that the file exists when node-lintspaces looks for it (if I remove the file from the dependency, I get Error: The config file "./node_modules/@myorg/code-standards/.editorconfig" wasn't found.).

I also tried renaming my .editorconfig file to .nonstandardname and passing in editorconfig: '.nonstandardname', and everything works as expected.

Any ideas why the file at ./node_modules/@myorg/code-standards/.editorconfig wouldn't work?

localjo commented 8 years ago

Seems that the issue is related to the .editorconfig being in a subdirectory. If I create /testconfig/.editorconfig and pass in editorconfig: './testconfig/.editorconfig' none of the rules seem to apply either.

schorfES commented 8 years ago

Have you tried to build your path with path.join(__dirname, 'subdir', '.editorconfig')?

localjo commented 8 years ago

Nope. I can experiment with that. It's not clear exactly what I should be passing in there. But the file is found if I use a path relative to my project root; editorconfig: './node_modules/@myorg/code-standards/.editorconfig', the settings just don't get loaded. I suspect the problem is somewhere on these lines: https://github.com/schorfES/node-lintspaces/blob/master/lib/Validator.js#L146-L152, but testing out changes to that file is taking me awhile to set up, since it's a deep dependency for me.

localjo commented 8 years ago

Using editorconfig: path.join(__dirname, 'testconfig', '.editorconfig'), I get the same results; the file is found, but the settings in it don't get loaded.

localjo commented 8 years ago

The problem might be with the editorconfig.parse() function. Trying to get into inception mode to debug.

schorfES commented 8 years ago

These lines belong to the editorconfig api. Maybe a security issue in node.js or your OS...?

localjo commented 8 years ago

Looks like my code ends up calling editorconfig.parse('/site/assets/scripts/index.js', { config: './testconfig/.editorconfig' });.

localjo commented 8 years ago

And editorconfig.parse returns {}, so the issue is over there. I'll move the discussion there. Thanks.

localjo commented 8 years ago

Actually, it seems like this might be related to using an outdated version of the editorconfig core: https://github.com/schorfES/node-lintspaces/blob/master/package.json#L48

schorfES commented 8 years ago

This version is outdated but currently required. Updating to the latest verison of editorconfig-core will bring breaking changes to the API of lintspaces itself. All in all it won't work synchronous anymore...

localjo commented 8 years ago

Tracked down my issue specifically to this line; https://github.com/editorconfig/editorconfig-core-js/blob/50e0dba81b2f7f3e9ea4f701f2c65dd3f482cd4c/editorconfig.js#L68

That's mistakenly(?) appending the .editorconfig's path prefix to the match against the linted file's path. Maybe that's how it's supposed to work, so that an .editorconfig file only applies to the files inside of the same directory as it. In the case of node-lintspaces, or gulp-lintspaces, it seems that the .editorconfig rules should be applied to all of the gulp.src files. But maybe that's a misuse of the editorconfig core. Either way, I'm not sure how to go about resolving the issue. Are you planning to update node-lintspaces to use the latest version of editorconfig-core-js at some point in the future, or will everything I'm suggesting require manual forks?

schorfES commented 8 years ago

Yes, I think about updating to the latest version, but as written before it will bring a complete api change to lintspaces which will result in a new major version number. So the new release may take a while...

localjo commented 8 years ago

Understandable, and no pressure. Just wanted to get an idea of where you see this heading. Thanks for the info, and more importantly, thanks for your work on this project. :)

schorfES commented 8 years ago

Thank you for using this project :heart: