Don't lint non-angular files
ptarjan opened this issue · 18 comments
I tried using your plugin, but part of my repo is angular and another part is just normal JS files. Can you only apply your rules if there is a angular.module() in the chain or something smart?
What kind of false positives do you have?
Tons. Here are a few samples:
4:1 error You should use the $window service instead of the default window object angular/window-service
114:7 error You should use the "error" method of the AngularJS Service $log instead of the console object angular/log
19:11 error You should use the toJson method instead of JSON.stringify angular/json-functions
674:26 error You should not use directly the "undefined" keyword. Prefer angular.isUndefined or angular.isDefined angular/definedundefined
244:33 warning You should use angular.element instead of the jQuery $ object angular/angularelement
25:15 error You should use the angular.isArray method angular/typecheck-array
It's not possible to determine which files you want to lint. This is a personal preference.
If your code base separates the angular part and the non-angular part, you could implement this yourself though.
Assuming you have the following layout
app/
|- angular-app/
| `- .eslintrc
|- non-angular/
| `- .eslintrc
`- .eslintrc
Where app/.eslintrc contains your common settings, app/non-angular/.eslintrc contains your non-angular specific rules and app/angular-app/.eslintrc contains:
plugins:
- angularI use a similar approach to distinguish between the rules for my configuration files (gulpfile, Karma configuration, etc.) and my AngularJS source files.
I often intermix pure JS modules in the same dir as my angular services or controllers.
Could this plugin just make sure there is a user of angular.module in the file before it reports anything?
I think it is not possible to detect code inside angular.module, because developers can use AMD or CJS module.
We can define the implementation of the AngularJS component in a file, and include it in the AngularJS architecture (via filter/directive/service/controller..) in another file.
In the first file, we do not have any angular.module call, so it is not possible to detect AngularJS code in this way.
Can you only lint inside a angular.controller and angular.service etc.?
Sent from my iPhone
On Oct 8, 2015, at 2:29 AM, Emmanuel DEMEY notifications@github.com wrote:
I think it is not possible to detect code inside angular.module, because developers can use AMD or CJS module.
We can define the implementation of the AngularJS component in a file, and include it in the AngularJS architecture (via filter/directive/service/controller..) in another file.
In the first file, we do not have any angular.module call, so it is not possible to detect AngularJS code in this way.—
Reply to this email directly or view it on GitHub.
As I tried to explain I think it is not possible. For exemple, imagine I have an ES2015 service class in a service.js file :
export class Service {}And in other file, I register this service into Angular via an application.js file :
import {Service} from 'myModule'
angular.module('Service', Service)How can we detect, in the service.js, that the class is in fact an AngularJS Service component ? I think it is impossible.
Manu
I agree. JavaScript is Turing complete so you can't know if something will be registered as a controller unless you execute it. But as with all static analysis, you do your best. My vote is to err on the side of false negatives not false positives for a linter. So only yell if you are sure it is a controller or service or whatever and the slowly get smarter about detecting them.
Sent from my iPhone
On Oct 11, 2015, at 6:35 AM, Emmanuel DEMEY notifications@github.com wrote:
As I tried to explain I think it is not possible. For exemple, imagine I have an ES2015 service class in a service.js file :
export class Service {}
And in other file, I register this service into Angular via an application.js file :import {Service} from 'myModule'
angular.module('Service', Service)
How can we detect, in the service.js, that the class is in fact an AngularJS Service component ? I think it is impossible.Manu
—
Reply to this email directly or view it on GitHub.
We could add a global setting and turn off the rules based on some heuristics. The detection might be incomplete, but it's a tradeoff.
"settings": {
"angular/detect-angular-context": ["module-setter"]
}
It think the simplest way it to look for a module getter or setter and then use the rules for the complete file. It might be also possible to check all parent nodes for a certain node, but I pretty sure this will cause performance issues. A file based approach is simpler.
I think it is a per rule setting which ones should be enabled.
I agree with @ptrarjan that the following errors should only be detected when inside of an AngularJS component:
4:1 error You should use the $window service instead of the default window object angular/window-service
114:7 error You should use the "error" method of the AngularJS Service $log instead of the console object angular/log
However, these rules could still apply when used outside of an AngularJS component. This is purely a preference. These rules can be disabled using /* eslint-disable angular/<rule_name> */.
19:11 error You should use the toJson method instead of JSON.stringify angular/json-functions
674:26 error You should not use directly the "undefined" keyword. Prefer angular.isUndefined or angular.isDefined angular/definedundefined
244:33 warning You should use angular.element instead of the jQuery $ object angular/angularelement
25:15 error You should use the angular.isArray method angular/typecheck-array
I personally don't have experience yet using ES6 specific syntax such as classes combined with AngularJS. ngAnnotate has support for ES6 and TypeScript. I think the 'ngInject'; annotation can be used to detect services or controllers from classes.
Another solution could be a disabling of the entire plugin from within a file via inline comments, but it seems to be not supported by eslint yet (eslint/eslint#3419, eslint/eslint#2118)
... or the angular components could be ignored using some filename conventions.
"settings": {
"angular/file-name-pattern": "/\w+\.(module|service|controller|directive|filter)\.js/"
}
Another suggestion. We provide different configurable heuristics for detecting angular files.
"settings": {
"angular/only-lint-matching-files": {
// configured heuristic
}
}
Ideas for such options...
"code-patterns": ["module-setters", "module-getters", "angular-object"]["module-setters"]- presence of a module setter["module-getters"]- presence of a module getter["angular-object"]- any use of the angular object
"file-name-patter": ["\.(module|service|controller|directive|filter)\.js$"]
If we find better ways to detect angular files we can easily add an options to this list.
The "code-pattern" option will cover the usecase of @ptarjan but it is also possible to write angular components without any reference to the framework with the "file-name-pattern" option.
I've added a proof of concept to my fork (here), which kinda allows to control whether or not plugin rules will be executed. It looks hacky, but might work. Basically it uses local variable to block rule execution from within a plugin, it resets the variable after each file ends and updates variable value when rule is enabled again. The following rule should block all angular rules for for current file:
/* eslint "angular/disable-plugin": 2 */
It does not work in alignment with ESLint API and may lead to bugs in plugin and maybe even ESLint, I agree if it won't end up in the plugin, but is one of the alternatives.
@mradionov Nice. Your proof of concept seems to work, but as you already said, it is a little hacky and I fear it may be hard to predict side effects.
I took a part of your approach to disabled rules by a filename pattern as another proof of concept.
If anybody is interested, I wrote a plugin for ESLint, which allows to disable plugins per file via inline comments https://github.com/mradionov/eslint-plugin-disable
@mradionov Nice! I think this is the best solution by far.
I'm closing this issue, because I think this fixes it. I don't think it should be part of an AngularJS plugin to handle this.
If anyone disagrees, just reopen it.
@remcohaszing I also like the solution and we should mention it in the docs, but I also think that some convention based approach is necessary, if a project has a lot on non angular resources.