Added support for extra file extensions #96

Closed
wants to merge 5 commits into from

4 participants

@stefounet

Hi Brent

Here is the extra ext option we discussed.

@brentlintner

Sorry to nitpick, but does this line not seem unnecessary? I.e. When extraExtensionList gets passed into hint.hint (line 138), it seems it's already handled internally, regardless of it's type or value (which I would expect, IMO).

It handles the case where --extra-ext is added with no file extension.
In this case, options["--extra-ext"] gives a boolean (true)

Oh crap.. haha. Right (just checked it out as well). Totally slipped my mind the two use cases where it may be true OR false(y). My bad!

@brentlintner

I think line 76 can be omitted in this case (seems like it could be merged into this line). I also think the . should be inside the (js portion of the regex, so you don't need to replace . with an empty string at the end, and it can support stuff like "Jakefile" as a file "extension". I.e... something like this?... :-)

var regExtension = new RegExp("(\.js" + (extraExtensionList ? "|" + extraExtensionList.replace(/,/g,"|") : "") + ")$";

Line 76 is here to handle the case where _collect would be called without this fourth arg but, il can be omitted.

Dots and space are replaced in the regex because there's is a dot for every extension and it's handled by the first \..
Your regexp doesn't match extension list like this : ".json, .txt" because dots are not escaped and space between extensions too.

So you're right, this code doesn't support files with no extensions but it does the main part of the job. We should handle the case of files with no extension later.

I will create an issue about handling files with no extensions (good point, should be put aside).

One last thing (sorry about this, lol). I just noticed this is happening inside _collect (every time it recurses). You also don't pass it to the recursive call on line 80. The regex creation will only be run once if done inside the public hint method, and the regex matcher is passed to _collect.

Yep, you're right. I didn't write a test with directory and extra file extensions ... I fix this asap

@brentlintner brentlintner and 1 other commented on an outdated diff Mar 27, 2012
@@ -7,3 +7,4 @@ Options:
--reporter custom reporter
--jslint-reporter use a jslint compatible xml reporter
--show-non-errors show additional data generated by jshint
+ --extra-ext comma-separated list of file extension to use (.js is default)

LOL... This seems so minute to bring up (and cause a rebase..)... but, perhaps extension should be pluralized?

yep, sure !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brentlintner brentlintner commented on the diff Mar 27, 2012
README.md
@@ -48,6 +48,12 @@ Specify custom lint options (see [example/config.json](https://github.com/jshint
Note: This bypasses any .jshintrc files.
+## File Extensions
+
+Default extension for files is ".js". If you want to use JSHint with other file extensions (.json), you need to pass this extra extension as an option

Perhaps a : or . at the end of this sentence? (not a big deal, but thought I would mention, for consistency)

idem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@niknah

When you type "jshint xxx.txt", it should error instead of doing nothing and failing silently.

Took me a while to figure out that it only took .js

@stefounet

Yep, you're right ! It also took me a while to find out why my json file wasn't parsed ...
We should add a message to show ignored files during the collect phase.

@brentlintner

@stefounet @niknah Cool, thanks for that. I created a separate issue for that. https://github.com/jshint/node-jshint/issues/97

@brentlintner

Also, there appear to be some lint errors (via the jake lint task) in this branch.

@smagch

I hope jshint doesn't sniff file extensions. I'm personally coding cli which has no extension and start from #!/usr/bin/env node. I'm ending up with

mv ./bin/jscat ./bin/jscat.js
jshint ./bin/jscat.js
mv ./bin/jscat.js ./bin/jscat
@brentlintner

@smagch It has been a pain point, but what other way to identify the file (cross platform) without picking up all files? #! is not 100% reliable (IMO), but may be a band aid solution.. however it does not (fully) solve the problem (i.e. it won't match a file like this). There is an open issue here for that, open to suggestions! :-)

@brentlintner

Thanks again. Published v0.6.2 with this (after rebasing, fixing some conflicts, and re-testing). I also squashed the commits into one (for succinctness). Hopefully that was ok. Let me know if there are any discrepancies!

86f618b

@stefounet
@chadaustin chadaustin pushed a commit to imvu/node-jshint that referenced this pull request Mar 13, 2013
@valueof valueof Make sure JSHint parses getters/setters as function expressions and n…
…ot declarations (#96)
16230ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment