Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ESLint and fix a few formatting errors #33

Closed
wants to merge 4 commits into from

Conversation

pdehaan
Copy link
Collaborator

@pdehaan pdehaan commented Jul 23, 2016

Currently ✖ 2 problems (0 errors, 2 warnings).

Once we land this and get the ESLint hook passing, we may want to add a npm script pretest hook that runs "pretest": "npm run lint" so ESLint will run automatically every time you run $ npm test or the build runs on Travis-CI. Done.

How to:

$ npm run lint

> blok@1.0.0 lint /Users/pdehaan/dev/github/mozilla/blok
> eslint .


/Users/pdehaan/dev/github/mozilla/blok/js/background.js
  185:11  warning  'actionPerformed' is defined but never used  no-unused-vars

/Users/pdehaan/dev/github/mozilla/blok/js/canonicalize.js
  28:7  warning  'isIP4Decimal' is defined but never used  no-unused-vars

✖ 2 problems (0 errors, 2 warnings)

r? @groovecoder

@@ -0,0 +1 @@
js/foundation.min.js
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want to commit 3rd party code in the repo, versus use something like npm or bower.

@groovecoder
Copy link
Member

Thanks for the tooling PR!

Based on feedback from other Mozilla webdevs, I had planned to try standard for coding style enforcement. Are these ESLint rules more aligned with other add-ons and Mozilla code-bases?

@groovecoder
Copy link
Member

Hmm ... needs a rebase now ...

@groovecoder
Copy link
Member

Superseded by #39 which includes the rebase.

@pdehaan
Copy link
Collaborator Author

pdehaan commented Jul 25, 2016

Are these ESLint rules more aligned with other add-ons and Mozilla code-bases?

We have no conventions or shared Mozilla ESLint config [that I'm aware of]. It's all Wild West based on repo owner's preferences. I haven't used Standard in a while, but I'll see about a follow-on PR that tries adding eslint-config-standard.

@pdehaan
Copy link
Collaborator Author

pdehaan commented Jul 25, 2016

Turns out injecting standard was pretty trivial by adding the eslint-config-standard config (and a couple plugins) as dev dependencies:

$ npm i eslint-config-standard eslint-plugin-{standard,promise} -D`

Then tweak the ESLint rules to extend the standard config:

extends:
  - eslint:recommended
  - standard

Finally run $ npm run lint -- --fix to run ESLint and auto-fix any auto-fixable errors:

$ npm run lint

> blok@1.0.0 lint /Users/pdehaan/dev/github/mozilla/blok
> eslint .

/Users/pdehaan/dev/github/mozilla/blok/js/background.js
    9:5   error    Identifier 'current_active_tab_id' is not in camel case          camelcase
   10:5   error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase
   11:5   error    Identifier 'current_active_origin' is not in camel case          camelcase
   12:5   error    Identifier 'blocked_requests' is not in camel case               camelcase
   13:5   error    Identifier 'blocked_entities' is not in camel case               camelcase
   14:5   error    Identifier 'allowed_requests' is not in camel case               camelcase
   15:5   error    Identifier 'allowed_entities' is not in camel case               camelcase
   16:5   error    Identifier 'total_exec_time' is not in camel case                camelcase
   17:5   error    Identifier 'reasons_given' is not in camel case                  camelcase
   49:5   error    Identifier 'current_active_origin' is not in camel case          camelcase
   50:5   error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase
   56:29  error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase
  172:5   error    Identifier 'current_active_tab_id' is not in camel case          camelcase
  179:11  warning  'actionPerformed' is defined but never used                      no-unused-vars
  179:30  error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase
  180:29  error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase
  181:29  error    Identifier 'current_origin_disabled_index' is not in camel case  camelcase

/Users/pdehaan/dev/github/mozilla/blok/js/canonicalize.js
   1:70  error    Unnecessary escape character: \           no-useless-escape
   2:45  error    Unnecessary escape character: \           no-useless-escape
   3:71  error    Unnecessary escape character: \           no-useless-escape
  19:62  error    Unnecessary escape character: \           no-useless-escape
  26:7   warning  'isIP4Decimal' is defined but never used  no-unused-vars

/Users/pdehaan/dev/github/mozilla/blok/js/lists.js
  37:12  error  Identifier 'category_name' is not in camel case  camelcase
  39:9   error  Identifier 'entity_count' is not in camel case   camelcase
  41:25  error  Identifier 'entity_count' is not in camel case   camelcase
  44:16  error  Identifier 'entity_name' is not in camel case    camelcase
  47:18  error  Identifier 'main_domain' is not in camel case    camelcase
  50:15  error  Identifier 'domains_count' is not in camel case  camelcase
  52:31  error  Identifier 'domains_count' is not in camel case  camelcase
  53:37  error  Identifier 'main_domain' is not in camel case    camelcase

✖ 30 problems (28 errors, 2 warnings)

I may let somebody else pull the trigger on this, since it looks like somebody will better domain knowledge than me to refactor the variable names into camelcase (or disable the Standard camelcase rule, if appropriate).


(Without running with the --fix flag, you should get ✖ 70 problems (68 errors, 2 warnings) from ESLint after switching to standard.)

@pdehaan
Copy link
Collaborator Author

pdehaan commented Jul 25, 2016

(Moved standard conversation to #40 instead of continuing in a closed PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants