Implement style guide / stick to it

Issue #20 closed
Zack Lovatt created an issue

No description provided.

Comments (20)

  1. Remco Janssen

    Shall we just use that one then? My only concern is the dropping of semicolons as they are still needed in specific cases.

  2. Rune Gangsø

    Yeah, there is some minor cases where semicolons are needed, but it should work without most of the time, at least in new versions of JS, but I'm not sure if AE/ES3 is more picky about it.

  3. Zack Lovatt reporter

    For what it's worth, losing semicolons is the only thing I disagree with here.

    Stylistically, the rest doesn't really matter -- but losing semicolons is the biggest thing that has the potential to actually affect the code. Telling contributors to just never use semicolons and hope that "it should work most of the time" seems unnecessarily risky / adds extra work to later find and debug.

  4. Rune Gangsø

    Doing a quick test it looks no semicolons mostly works on the existing codebase, at least there is no syntax errors, except for it choking when there is no semicolons after break in switch..case.

    I've been coding in after effects without semicolons for a while, in big and small projects, and never had any issues. There is only a few cases where semicolons are "needed" (take a look at what standardjs says about it), but those are easily avoided and usually only come up if you are writing "smart" code, so I've never had to use any.

    I've not been using switch...case so it not working there is new to me.

    Because of the size of the codebase, and it already being hard to test everything, I think it might be better to leave the semicolons alone. It's a bit sad, as I think it's much nice to write and read without them, even if there is those few "issues" with it.

  5. Remco Janssen

    It really comes down to personal preference and habit. Much like spaces vs tabs and tab size.

    I'm fine with whatever we decide, as long as ESLint is super specific, so I don't have to think about the style as it probably goes against my own style (or habit).

    So with multiple people working on a codebase we'll all have different habits and the linter needs to be really verbose.

    Complimentary the style should be mentioned in the contribution guide (#40)

  6. Rune Gangsø

    All linting errors and warnings are now either fixed or silenced.

    The silenced ones are:

     ./lib/main.js
      154:6  warning  Blocks are nested too deeply (5)  max-depth
    ./lib/select.js
      80:14  warning  Function has a complexity of 38  complexity
     ./lib/ui/container.js
      297:13  warning  Method 'addSlider' has too many parameters (5). Maximum allowed is 4  max-params
    
  7. Remco Janssen

    Great! @runegan I noticed you added some todo's in the code. We should create new issues for them.

    @zlovatt @rafikhan Approved to merge and resolve this issue?

  8. Remco Janssen

    Cool, guess we can merge then? @runegan

    Just to be clear/for future reference: aeQuery does require semicolons!

  9. Rune Gangsø

    Merge branch 'linting' into develop

    • linting: Rename renderqueue.unqueue_all to unqueueAll linting: Fix and silence errors Linting: Fix no-use-before-define errors lint: Update indent rules linting: Manually fix errors linting: run lint-fix Pipeline: run linter before building package Add linting rules update eslint add editorconfig Add .eslintignore Add pre-commit Add eslint and gulp lint task

    closes: #20

    → <<cset b1d899839e7b>>

  10. Log in to comment