Implement style guide / stick to it

[Former user]

No description provided.

[Rune Gangsø]

What style guide should be used? And how should it be enforced? My vote is for jQuery style guide and eslint for enforcing it.

I would like to modify the style slightly though. Using single quotes and not using any semicolons. Take a look at the jsxbin repo on github to see how that looks.

[Rune Gangsø]

See this branch for how jquery style looks on this project

[Former user]

Beautiful

[Remco Janssen]

Agree on ESLint to enforce style. I like the jsxbin example.

@runegan Do you already have an ESLint config for that?

[Rune Gangsø]

@Klustre I have eslint-config-runegan, which is what the jsxbin repo is using.

[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.

[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.

[Former user]

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.

[Remco Janssen]

I agree with @zlovatt

[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.

[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)

[Rune Gangsø]

Add linting rules

See #20

→ <<cset afa73962dc90>>

[Rune Gangsø]

I have run the eslint fix tool (4f165b0) and gone through and fixed trivial errors it didn't fix (a6addc8), but a few still remain.

Mostly: aeq' was used before it was defined no-use-before-define caused by the var aeq = ( function () ) closures.

Those closures are removed by gulp for some reason, so wondering if we need them at all.

Full list of errors: https://bitbucket.org/motiondesign/aequery/addon/pipelines/home#!/results/117

[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

[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?

[Former user]

Looks good!

[Remco Janssen]

Cool, guess we can merge then? @runegan

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

[Rune Gangsø]

@Klustre That is correct.

Merging linting into develop and closing this issue.

[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>>