Skip to content
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

Hound does not respect tests/.jshintrc #889

Open
elwayman02 opened this issue Aug 20, 2015 · 17 comments
Open

Hound does not respect tests/.jshintrc #889

elwayman02 opened this issue Aug 20, 2015 · 17 comments

Comments

@elwayman02
Copy link

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.

@alexstophel
Copy link

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.

@thorncp
Copy link
Contributor

thorncp commented Aug 22, 2015

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?

@elwayman02
Copy link
Author

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
Copy link
Author

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

@tf
Copy link

tf commented Aug 25, 2015

+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
Copy link

tf commented Aug 25, 2015

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.

@teoljungberg
Copy link
Contributor

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

@elwayman02
Copy link
Author

@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.

@teoljungberg
Copy link
Contributor

@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.

@tf
Copy link

tf commented Aug 27, 2015

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

@thorncp
Copy link
Contributor

thorncp commented Aug 27, 2015

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.

@elwayman02
Copy link
Author

@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.

@arxpoetica
Copy link

arxpoetica commented Oct 28, 2016

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

@gylaz
Copy link
Member

gylaz commented Sep 14, 2017

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 gylaz closed this as completed Sep 14, 2017
@arxpoetica
Copy link

My comment was about ESLint which also requires/supports hierarchical config files. Should I reopen the original ESLint ticket for that (#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.

@gylaz
Copy link
Member

gylaz commented Sep 17, 2017

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 gylaz reopened this Sep 17, 2017
@elwayman02
Copy link
Author

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

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

No branches or pull requests

7 participants