Fix Lint Errors for TravisCI #601
Conversation
|
Changes Unknown when pulling 3644792 on davidmsibley:master into ** on UW-Madison-DoIT:master**. |
|
📓 resolves #599 |
|
I can't seem to find the comment, but I got the email, @ChristianMurphy, about eslint tooling, and dang do I wish I knew about https://github.com/IanVS/eslint-nibble before this! I had a watch running eslint every two minutes, and a live-server continually refreshing the results of the run. Kinda hacky but it worked for this. I had linter running in Atom though, which made fixing the individual errors much easier. |
| @@ -102,24 +102,35 @@ | |||
| ], | |||
| "rules": { | |||
| "angular/angularelement": "error", | |||
| "angular/di": "error", | |||
| "angular/di-order": "error", | |||
| "angular/di": "warn", | |||
There was a problem hiding this comment.
Google's standard is to use function syntax rather than array syntax for dependency injection. However, IIRC function syntax was not always available, and I'm guessing that we kept using array syntax due to momentum.
We believe that eventually we'd like to switch to function syntax to comply with Google's standard. We should backlog a discussion about this. @ChristianMurphy mentioned that array syntax is actually useful for minifiers to be able to minify DI variables.
| "angular/di": "error", | ||
| "angular/di-order": "error", | ||
| "angular/di": "warn", | ||
| "angular/di-order": "warn", |
There was a problem hiding this comment.
This is mostly stylistic, so it can stand to be a warning for now. We intend to bite the bullet at some point and flip this back to error.
| "angular/di-unused": "error", | ||
| "angular/document-service": "error", | ||
| "angular/log": "error", | ||
| "angular/module-dependency-order": "error", | ||
| "angular/no-service-method": "error", | ||
| "angular/no-services": "error", | ||
| "angular/no-services": "warn", |
There was a problem hiding this comment.
We ran into a legacy usage of $q.all() and determined it was outside the scope of this pull request to fix it. We should backlog that fix and flip this back to error as soon as it's done.
package.json
Outdated
| "max-len": [ | ||
| "error", | ||
| { | ||
| "code": 120, |
There was a problem hiding this comment.
This one's my own subjective opinion, I welcome a challenge to this. I feel that a line length of 80 is far too short for programming today. In recent years we've shifted to value descriptive function and variable names. Combine that with the base level of indentation in files, and I believe this rule becomes unnecessarily restrictive to developers. I feel that upping the line length by 40 to 120 gives developers room to breathe, but keeps files to a reasonable width.
There was a problem hiding this comment.
I'd favor not making an exception around line length. descriptive function and variable names are good -- and refactoring to better abstractions when 80 characters feels restrictive is good too.
I agree it's a question of style. The meta-value in not making exceptions about style is high. The first rule of code-style-club is don't talk about code-style-club. -0.9.
There was a problem hiding this comment.
good enough for me. This was one of the first rules I changed to drop the number of errors I had to contend with. I'm going to drop it back down to 80 locally and see if fixing that is feasible now that I fixed everything else.
There was a problem hiding this comment.
I'm in favor of the expanded line width. Given the size of modern monitors, and the fact that we don't code on punch cards anymore - my .02 is that an 80 character limit feels archaic.
There was a problem hiding this comment.
I'm willing to put in a little extra effort to adhere closely to the Google spec now. If we feel it's a hindrance in the future, we'll be better informed to make that decision then.
package.json
Outdated
| "no-new-symbol": "off", | ||
| "no-this-before-super": "off", | ||
| "no-invalid-this": "off", |
There was a problem hiding this comment.
This is a rule in base eslint:recommended that causes some trouble when used with Angular. It's common practice to use this within the controller function.
There was a problem hiding this comment.
Interesting I wasn't aware the project is using the this controller convention.
the angular/controller-as rule in eslint-plugin-angular might be a good addition.
There was a problem hiding this comment.
Maybe I jumped the gun on that. @thevoiceofzeke any thoughts on this? A quick ctrl-f seems to imply we assign scope variables in controllers far more often than we use this
There was a problem hiding this comment.
It's a mixed bag throughout the project. My personal preference is to use controllerAs: 'vm' and var vm = this, avoiding $scope and this entirely (something I picked up from the johnpapa Angular style guide. This project is a long, long way from that though.
In general I'm in favor of avoiding this inside angular controllers.
There was a problem hiding this comment.
@davidmsibley @thevoiceofzeke @apetro
If you are targeting johnpapa's styleguide I would recommend removing all the custom angular configuration I put in place and instead add eslint-config-angular.
There was a problem hiding this comment.
I'd be up for that, but possibly as its own effort. I dropped that in to my local and it looks like we're about 350 errors away from johnpapa's ideal.
package.json
Outdated
| "no-var": "off", | ||
| "prefer-rest-params": "off", | ||
| "prefer-spread": "off", | ||
| "require-jsdoc": "error", |
There was a problem hiding this comment.
This is standard, but I added it here to show the difference between just having JSDoc comments, and making sure JSDoc comments are perfect.
There was a problem hiding this comment.
personally I prefer to keep the rules section set only with rules that are being customized, but thats just me. 🤷♂️
package.json
Outdated
| "no-var": "off", | ||
| "prefer-rest-params": "off", | ||
| "prefer-spread": "off", | ||
| "require-jsdoc": "error", | ||
| "valid-jsdoc": "warn", |
There was a problem hiding this comment.
Having correct, well formatted docs is a goal of ours, but outside the scope of this pull request. I wasn't confident in my ability to comment the code correctly. We should work on cleaning up the comments bit by bit, and flip this back to error.
There was a problem hiding this comment.
@davidmsibley If complete and accurate JSDocs are a goal, eslint-plugin-jsdoc can offer some additional suggestions past what ESLint offers by default.
@davidmsibley I deleted the comment after I realized all the work was already done. |
|
@ChristianMurphy wrt #601 (comment) , it's just as much that we (and I'll take responsibility, I) communicated too late about the intention to do this work as it is you posted your solution too late. I think there's going to be a learning curve, a transition period in how tactical plans are created and communicated as we all learn to treat |
1 similar comment
Broad but hopefully thin changeset for fixing the Lint errors we should fix, and being warned about the ones that we can fix as we go. The goal is to stay as close to Google's eslint standards as we can. Rule Overrides will be documented as line comments in the pull request.
Contributor License Agreement adherence: