#8035 Finalize frontend eslint/jscs setup

v1.4.0
closed
General
2016-02-05
2015-12-14
No

Finalize and cleanup our eslint/jscsrc setup.

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 attached - they need more work though)

Then apply these styles to [#7919].

2 Attachments

Related

Tickets: #7919
Tickets: #8039

Discussion

  • Heith Seewald

    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

  • Dave Brondsema

    Dave Brondsema - 2015-12-14
    • labels: --> sf-current, sf-4
     
  • Dave Brondsema

    Dave Brondsema - 2015-12-18

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

     

    Related

    Tickets: #7919

  • Dave Brondsema

    Dave Brondsema - 2015-12-18
    • 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)
    
     

    Related

    Tickets: #7919

  • Heith Seewald

    Heith Seewald - 2015-12-18
    • status: open --> in-progress
    • assigned_to: Heith Seewald
     
  • Dave Brondsema

    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
     
  • Heith Seewald

    Heith Seewald - 2016-01-22
    • status: in-progress --> review
     
  • Heith Seewald

    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.

     
  • Dave Brondsema

    Dave Brondsema - 2016-01-25
    • status: review --> in-progress
    • Reviewer: Dave Brondsema
     
  • Dave Brondsema

    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.

     
    • Dave Brondsema

      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 ?

       
      • Heith Seewald

        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.

         
    • Heith Seewald

      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?

       
      • Dave Brondsema

        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.

         
    • Heith Seewald

      Heith Seewald - 2016-01-26
      1. I agree with allowing unused-vars as a means of identifying a function signature.
      2. Fixed ReferenceError in browser
      3. Now using the default eslint parser
        • needed to make adjustments to proptypes and defaultProps inside classes
      4. fixed inverted "Grouping threshold input box"

      force pushed to hs/8035

      (I still need to look at the one-var setting)

       
  • Heith Seewald

    Heith Seewald - 2016-01-27
    • status: in-progress --> review
     
  • Heith Seewald

    Heith Seewald - 2016-01-27

    force pushed to hs/8035

     
  • Dave Brondsema

    Dave Brondsema - 2016-02-05
    • status: review --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2016-04-11
    • Milestone: unreleased --> v1.4.0
     

Log in to post a comment.