New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for .jshintrc's in the directory being linted? #833

Open
domenic opened this Issue Jan 25, 2013 · 20 comments

Comments

Projects
None yet
@domenic
Copy link
Contributor

domenic commented Jan 25, 2013

Given:

lib/
  .jshintrc
  myFile.js
test/
  .jshintrc
  myTest.js

I tried

jshint lib test

but neither of my .jshintrcs were found. This is expected behavior, I guess, since the readme says it looks in the CWD. Still, it's pretty awkward: now I have to do

cd lib && jshint . ; cd ../test && jshint . ; cd ..

And I guess it would be impossible to have nested .jshintrcs.

Any chance on using the closest .jshintrc when linting a file? I know that's a pretty big and probably backward-incompatible change though, so if not, I understand.

@valueof

This comment has been minimized.

Copy link
Member

valueof commented Jan 26, 2013

Uh, I'd rather extend the default config to accept pre-directory entries:

{
    "global": { "strict": true },
    "test/**": { "strict": false }
}

Or something like that. But that'd be another ticket.

@valueof valueof closed this Jan 26, 2013

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Feb 11, 2013

I think there are other reasons to do this rather than multiple .jshintrc files for project (although that would be a nice side-benefit). As a developer, when I put a .jshintrc file above a .js file in my project, I'm saying that I want that rc file to apply to that js file - where I'm running jshint from isn't really relevant to me, I just want those files checked correctly, without having to worry about the cwd, or more significantly, where my editor is running when it's doing linting. I can't stress how annoying it is to have vim throw a bunch of errors at me, just because I forgot to cd before opening a .js file. Then I've got to close it, cd, and re-open it. Makes all my nice FuzzyFinder fast open shortcuts kinda useless.

If you're willing to reconsider the feature, I'd be happy to put together a patch for this. I did a bit of research, and while looking up a .jshintrc per file is a bit more work, the drawbacks could be minimized by memoizing the .jshintrc file lookup by target path. I think most of the work would come from re-working the config tests.

TL;DR: Lookup relative to linted file makes life easier, better represents developer intent, & shouldn't add significant overhead to the linting process.

@metamatt

This comment has been minimized.

Copy link

metamatt commented Feb 12, 2013

I'd like this too.

@spmason

This comment has been minimized.

Copy link

spmason commented Mar 5, 2013

+1. It would really help for example when you've got a web and a node directory that you want to apply different rules to (like browser:true or globals), but have some common settings in the parent directory that apply to both (like the strict rules)

@dominicbarnes

This comment has been minimized.

Copy link
Contributor

dominicbarnes commented Mar 5, 2013

Perhaps the config is determined by a sort of "inheritance" pattern? Maybe the directory of the file is checked first, and then the script traverses up the directory tree and gathers all the available configs in the hierarchy together. Then it can either go top-to-bottom (closer to root takes precedence) or bottom-to-top. (closer to target file takes precedence)

With the "bottom-to-top" approach, you could easily set "global" config rules for a project, and allow specific directories to override only as-needed. Certainly makes pinning down exactly what rules are applied where a bit trickier, but that complexity is only introduced by conscious effort on the part of a project.

@spmason

This comment has been minimized.

Copy link

spmason commented Mar 5, 2013

Yes I was imagining something like that, I think you'd want the configs closer to the target file to take precidence really.

The "pinning down errors" issue is also offset by the flexibility, and the fact that the rules for determining the config are relatively easy to understand/explain

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Mar 5, 2013

I'm actively working on a pull request that should handle this. The goal is to work exactly like the current implementation, expect instead of searching for .jshintrc relative to the current directory, it searches relative to the parent directory of the file being checked. Hopefully I'll have something ready within the next week.

MattCheely added a commit to MattCheely/jshint that referenced this issue Mar 7, 2013

Search for .jshintrc relative to the file being linted.
This patch updates the logic that looks for a .jshintrc file to
use to search relative to the file being linted rather than the
current working directory. If no file is found, config still
falls back to ~/.jshintrc.

This behavior should better reflect user intent when linting
files, regardless of where jshint (or the editor leveraging
it for checks) is run from. Since config lookup is now per-file
instead of per-run, the internal findFile method has been
memoized to avoid redundant searching.

References:
  Fixes jshintGH-833
@crossman

This comment has been minimized.

Copy link

crossman commented Mar 21, 2013

Any chance of getting this pull request merged in?

I'm currently working on a node application that hosts some public javascript files using jQuery. It doesn't make sense to just consider jQuery a global for the node files. And there are quite a few public files so configuring via comments just seems tedious.

@necolas

This comment has been minimized.

Copy link

necolas commented Apr 15, 2013

I'm kind of looking for something like this too. I'd like to be able to specify some extra predef's for my test spec directory, without running the risk of allowing globals of describe, it, etc., to escape unnoticed in my app's JS files. I was hoping that adding a .jshintrc with just the extra predef entries would result in them being added to the default config inherited from the root.

What do you suggest if multiple option files are off the table?

outsideris added a commit to outsideris/jshint that referenced this issue Apr 15, 2013

Search for .jshintrc relative to the file being linted.
This patch updates the logic that looks for a .jshintrc file to
use to search relative to the file being linted rather than the
current working directory. If no file is found, config still
falls back to ~/.jshintrc.

This behavior should better reflect user intent when linting
files, regardless of where jshint (or the editor leveraging
it for checks) is run from. Since config lookup is now per-file
instead of per-run, the internal findFile method has been
memoized to avoid redundant searching.

References:
  Fixes jshintGH-833
@jamesarosen

This comment has been minimized.

Copy link

jamesarosen commented Oct 30, 2013

+1 for this concept. See, for example, https://github.com/emberjs/ember.js/blob/master/.jshintrc. That project defines all of the test-related globals for all files, which is slightly dangerous. I could easily see a programmer accidentally typing raises instead of raise in a library file or forgetting to declare a local start or stop variable.

For this particular project, the pattern-matched globals would work much better:

{
    "*": { "strict": true },
    "packages/*/tests/**": { "globals": [ "QUnit", "..." ] }
}

That leaves the question of whether to merge or replace those Arrays.

The "closest .jshintrc" version would require Ember to have about 22 .jshintrc files, with lots of duplication.

Still, I think the "closest jshintrc" version would work for the majority of projects. If you want to scope this issue down to just globals, you could have JSHint look up the closest "prereq" file. cf #839

@jamesarosen

This comment has been minimized.

Copy link

jamesarosen commented Oct 30, 2013

Another option that would work for our case and for ember/ember.js would be to allow extraGlobals to be passed as command-line arguments. Then, we would just run jshint multiple times:

jshint lib/foo.js lib/bar.js
jshint spec/foo_spec.js spec/bar_spec.js --extraGlobals="expect,it,describe"
@OliverJAsh

This comment has been minimized.

Copy link

OliverJAsh commented Jan 8, 2014

I have the same needs. I want to extend my JSHint options for my test directory so that I can define my test framework's globals.

@jamesarosen You might also want to define linting rules for a file glob pattern such as test/**/*.spec.js, in case you have files in that directory which won't be run under by a testing framework.

@xaka

This comment has been minimized.

Copy link

xaka commented Feb 27, 2014

I've just run into the same problem. I have a global .jshintrc with default options for all *.js files and I have app and test directories which have slightly different options, mostly i'm talking about globals, but there are other cases. It would be nice for children .jshintrc files to be merged into their parents. If it breaks a backward compatibility, we might consider adding an option to children files to switch to merging mode.

@icarus-to-the-sun

This comment has been minimized.

Copy link

icarus-to-the-sun commented May 17, 2014

So it says this is closed due to outsideris@7eb565f but I don't experience the behavior of .jshintrc files overriding the topmost .jshintrc. Am I missing something? How do we apply options on a per directory basis?

@outsideris

This comment has been minimized.

Copy link

outsideris commented May 18, 2014

@icarus-to-the-sun
it's not my commit. it iss MattCheely/jshint@571ba17 's
It's just referenced when I rebase it in my repository. sorry about it.

@valueof valueof reopened this May 23, 2014

@denis-sokolov

This comment has been minimized.

Copy link
Contributor

denis-sokolov commented Jun 25, 2014

I am very interested in merging behavior, perhaps with an opt-in key,
but how is that related to the original issue #833? Can we have a new issue for that, please?

@valueof

This comment has been minimized.

Copy link
Member

valueof commented Jul 6, 2014

@denis-sokolov Just open a pull request if you have code.

@valueof valueof added the P3 label Jul 6, 2014

@denis-sokolov

This comment has been minimized.

Copy link
Contributor

denis-sokolov commented Jul 7, 2014

I don't, I just wanted to help organizing issues.
You could concievably discuss all bugs and features in a single issue, of course.
It's not my business, of course, to comment on the issue organization.
I'll just shut up now. :)

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Search for .jshintrc relative to the file being linted.
This patch updates the logic that looks for a .jshintrc file to
use to search relative to the file being linted rather than the
current working directory. If no file is found, config still
falls back to ~/.jshintrc.

This behavior should better reflect user intent when linting
files, regardless of where jshint (or the editor leveraging
it for checks) is run from. Since config lookup is now per-file
instead of per-run, the internal findFile method has been
memoized to avoid redundant searching.

References:
  Fixes jshintGH-833
@OliverJAsh

This comment has been minimized.

Copy link

OliverJAsh commented Oct 30, 2014

AFAICS this was fixed by goonoo#2 and can be closed.

@condemoreloer

This comment has been minimized.

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