Apache Allura™ / Tickets / #8035 Finalize frontend eslint/jscs setup

Heith Seewald - 2015-12-14

Description has changed:

Diff:

--- old +++ new @@ -3,3 +3,5 @@ This can be a working standard that evolves over time -- but we should get something in place now so that we can start collecting feedback. *(Dave has a few tweaked config files locally)* + +Then appply these styles to [#7919].

Related

Tickets: ~~#7919~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2015-12-14

labels: --> sf-current, sf-4
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2015-12-18

[#7919] adds .jscsrc and .jshintrc but those can be changed or removed as needed.

Related

Tickets: ~~#7919~~

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Description has changed:

Diff:

--- old
+++ new
@@ -2,6 +2,6 @@

 This can be a working standard that evolves over time -- but we should get something in place now so that we can start collecting feedback.

-*(Dave has a few tweaked config files locally)*
+*(Dave has a few tweaked config files attached - they need more work though)*

-Then appply these styles to [#7919].
+Then apply these styles to [#7919].

Attachments has changed:

Diff:

--- old
+++ new
@@ -0,0 +1,2 @@
+.eslintrc (416 Bytes; application/octet-stream)
+.jscsrc (169 Bytes; application/octet-stream)

Tickets: ~~#7919~~

Heith Seewald - 2015-12-18

status: open --> in-progress

assigned_to: Heith Seewald
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-01-18

I think the Google style guide settings are pretty good, at least I like in terms of variable capitalization, where to put spaces, etc. A few things I'd probably tweak if it were all up to me:

longer line lengths. We use 119 for python.

allow strings to be either single or double quoted
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Heith Seewald - 2016-01-18

I agree with these. Anyone else want to weigh in on the eslint rules?

For reference:
Full list of options for eslint
Google style guide for javascript

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Heith Seewald - 2016-01-22

status: in-progress --> review
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Heith Seewald - 2016-01-22

Ready for review at hs/8035

For QA:
1. run npm install.
2. run npm run lint

most fixes are one per commit.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-01-25

status: review --> in-progress

Reviewer: Dave Brondsema
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-01-25

Can we have a separate .eslintrc for ES5 files? The .eslintrc file from master needs to be kept around in some fashion and used in def validate_js so that allura.tests.functional.test_static:TestStatic.test_static_controller and inline_js tests work correctly.

For the no-unused-var changes, I prefer to allow unused function params like e for event handlers, i for some loops, etc, as an indicator of what the function signature actually is even if they aren't used. This can be permitted with "no-unused-vars": [2, {"vars": "all", "args": "none"}],. What do you think?

In the browser, I get an error "ReferenceError: module is not defined".

Grouping threshold input box shows up at the wrong time, I think the !! -> Boolean change messed it up. Also its tooltip has backslashes in it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Dave Brondsema - 2016-01-25
  
  Also, for variable declarations, I'm not used to the "one-var" setting of each var on its own line, but I can get accustomed to it. However, that also permits vars to be declared further down in a scope (e.g. within a loop or within an if statement) which can cause accidental logic bugs since they are actually scoped to the whole function. How about we enforce vars-on-top ?
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Heith Seewald - 2016-01-27
    
    I agree. I added the "vars-on-top" -- it also encourages the use of "let" over "var" for block scoped variables. Good call.
    
    I could go either way on the “one var per line” rule. But I have read a few decent arguments in favor of using it.
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Heith Seewald - 2016-01-26
  
  Great feedback -- I'll respond to one at a time and go from there.
  
  Can we have a separate .eslintrc for ES5 files?
  
  Yes -- we should be able to do something like the following inside our package.json:
  
  "scripts": { ... "eslint": "eslint", "lint-es5": "eslint -c .eslintrc-es5 --ignore-path .eslintignore-es5 Allura/allura/public/**/*.es6.js || true", "lint-es6": "eslint -c .eslintrc-es6 --ignore-path .eslintignore-es6 Allura/allura/public/**/*.js || true", "lint": "npm run lint-es6 && npm run lint-es5" },
  
  Or if test_static_controller expects a regular .eslintrc file -- then we can use that naming convention for the es5 (or adjust the test).
  
  What do you think?
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Dave Brondsema - 2016-01-26
    
    test_static_controller calls validate_js which calls the lint command. It currently runs npm run eslint plus additional parameters. It'd be fine to specify the es5 file as one of the parameters there.
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Heith Seewald - 2016-01-26
  
  I agree with allowing unused-vars as a means of identifying a function signature.
  
  Fixed ReferenceError in browser
  
  Now using the default eslint parser
  
  needed to make adjustments to proptypes and defaultProps inside classes
  
  fixed inverted "Grouping threshold input box"
  
  force pushed to hs/8035
  
  (I still need to look at the one-var setting)
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Heith Seewald - 2016-01-27

status: in-progress --> review
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Heith Seewald - 2016-01-27

force pushed to hs/8035

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-02-05

status: review --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-04-11

Milestone: unreleased --> v1.4.0
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Apache Allura™

Forge software for hosting software projects

Milestone

Searches

Help

#8035 Finalize frontend eslint/jscs setup

Related

Discussion

Related

Related

Related