hound icon indicating copy to clipboard operation
hound copied to clipboard

Hound does not respect tests/.jshintrc

Open elwayman02 opened this issue 10 years ago • 17 comments

Currently Hound only supports a single JSHint config, even though standard usage in most products is to provide a different .jshintrc for tests than what's used for app code. This allows developers to declare globals that SHOULD throw warnings in app code (such as test framework methods) while ignoring them inside of tests. As far as I can tell, there is no way to configure Hound to respect the test config.

elwayman02 avatar Aug 20 '15 00:08 elwayman02

I think this is a great request, @elwayman02. I wouldn't mind trying my hand at implementing this as an enhancement if others closer to the project think it'd be a good idea.

alexstophel avatar Aug 20 '15 22:08 alexstophel

I don't think this is likely to happen soon.

Hound only knows about files that are included in the pull request or referenced in .hound.yml. In order to fully support this feature of JSHint, it would either:

  • Require access to the entire repository so it could walk the directory tree looking for .jshintrc files. We do not want to do this because the costs of changing the architecture are too high and we really like that we don't currently store users' code.
  • Use the GitHub API to find relavent .jshintrc files for a given file. We probably do not want to do this because we already have API usage issues and this would increase the number of requests we make. Caching across builds could be a way to get around this, but does kinda cross into the storing-user-code territory.
  • Allow specifying all config files in .hound.yml and grabbing them directly. We probably do not want to do this because we're in the process of simplifying configuration and this would be counter to that.

All that said, I do think this would be a great feature to support and would like to see it work. Is there another way I missed?

thorncp avatar Aug 22 '15 00:08 thorncp

I don't think it gets much simpler than specifying a linters name and it's config file(s). Those are literally the only options you should need, really.

elwayman02 avatar Aug 22 '15 00:08 elwayman02

That is, I think #3 is necessary but not the evil you think it is.

elwayman02 avatar Aug 22 '15 00:08 elwayman02

+1 for a feature like this. Currently no way of linting test files correctly. Listing multiple .jshintrc files in .hound.yml (option 3) seems like the best choice from my point of view.

tf avatar Aug 25 '15 11:08 tf

Taking a quick peek at the code: js files are in fact linted one by one. Once could extend RepoConfig#for to also take a target file name and return the linter config from the "closest" config file if multiple are registered. That would mimic the "directory walking" behavior of linters while still only depending on explicitly named config files in the repository.

tf avatar Aug 25 '15 12:08 tf

I don't see why your test code should other linting rules to it than your application code.

teoljungberg avatar Aug 26 '15 20:08 teoljungberg

@teoljungberg I've explained why in the OP. Often test code uses globals that do not (or should not) exist in app code. For example, many projects using QUnit utilize globals such as test or module. In order to pass JSHint, you usually add those to the predef array in jshintrc. These should NEVER be included in your application's linting, because you would want to catch erroneous uses of these undefined methods.

elwayman02 avatar Aug 26 '15 21:08 elwayman02

@elwayman02 I don't agree. Your test code should follow the same principles as your application code. If your test-code uses globals, they should be refactored away.

But, then again - I've never used QUnit and experienced this problem.

teoljungberg avatar Aug 27 '15 14:08 teoljungberg

All of these frameworks use globals to define the DSL. There is not good way of refactoring away describe and it...

tf avatar Aug 27 '15 14:08 tf

If there are multiple config files for a given file path, does JSHint merge them or pick the closest?

Edit: The JSHint docs say this1:

jshint will look for this configuration in a number of locations, stopping at the first positive match

Are y'all actively maintaining two separate, complete config files for production and test code?

Also note, this feature is documented under the CLI section, which Hound does not use. Any support for this feature would be a recreation of it within Hound.

thorncp avatar Aug 27 '15 16:08 thorncp

@thorncp Yes, most projects I have seen that use JSHint have app/.jshintrc and tests/.jshintrc. This seems to be industry standard from what I can tell, and is the default configuration for generator tools like Ember-CLI as well.

elwayman02 avatar Aug 27 '15 19:08 elwayman02

I've closed my prior ticket (which is the same issue, different reasons) and putting in my two cents here (copied from the other ticket):

Relevant comment 1: ESLint mentions that config files can have hierarchy and cascade. http://eslint.org/docs/user-guide/configuring#configuration-cascading-and-hierarchy This doesn't appear to work at all with Hound. Is there a reason this doesn't work?

Relevant comment 2: Node projects [are a use case where different config files are needed for different folders]. Don't enable, for example, browser on the server environment. But enable it for any client js. Vice versa, don't enable node in the client.

Thanks.

(Closed ticket: #1210.)

arxpoetica avatar Oct 28 '16 15:10 arxpoetica

There hasn't been any movement on this, and most of JS community seems to be using ESLint now. I'm going to close this.

gylaz avatar Sep 14 '17 23:09 gylaz

My comment was about ESLint which also requires/supports hierarchical config files. Should I reopen the original ESLint ticket for that (https://github.com/houndci/hound/issues/1210)?

Also, I don't know what you mean by "no movement" on this. Do you mean no movement by the Hound developers? As I understand it, it's out of the community's control, unless there's something I'm missing. I was waiting on your development team to (hopefully) work on it, so hearing it closed as "no movement" is disheartening and makes it sound like what you really mean is that the Hound/thoughtbot developers are just not planning on implementing this important feature.

If you don't plan on developing it for those reasons, please help us understand so we can make a more informed decision on whether to continue using hound or not based on this feature.

Thanks.

arxpoetica avatar Sep 17 '17 03:09 arxpoetica

Also, I don't know what you mean by "no movement" on this. Do you mean no movement by the Hound developers?

I mean by both the developers and no additional comments on this in almost a year (thus assuming it's not a pressing issue). I've re-read the comments again (and #1210) and it seems this is something we could/should support.

As I understand it, it's out of the community's control, unless there's something I'm missing.

Not entirely. This is an open source project.

I think it makes sense to re-open this issue.

gylaz avatar Sep 17 '17 04:09 gylaz

Personally, I've just given up on using Hound at this point. Issues like this are hard blockers to even consider it.

elwayman02 avatar Sep 17 '17 17:09 elwayman02