Skip to content
This repository has been archived by the owner on Mar 8, 2022. It is now read-only.

code check with eslint #52

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

code check with eslint #52

wants to merge 4 commits into from

Conversation

Eden-Sun
Copy link
Contributor

@Eden-Sun Eden-Sun commented Jan 18, 2017

replace jshint with eslint which is more modern for js codebase.

@calmh
Copy link
Owner

calmh commented Jan 18, 2017

Hi. Thanks, I'm sure these are good fixes. However I'm not really fond of a single PR containing bugfixes, formatting changes and switching out the CI system. In fact, I'm not sure why we need to change CI at all?

@Eden-Sun
Copy link
Contributor Author

Eden-Sun commented Jan 19, 2017

alright, i gonna separate them.
I switch to circle-ci because travis seemed to be disabled on this repo.

@Eden-Sun Eden-Sun force-pushed the eslint branch 2 times, most recently from 91ea2e5 to a343b81 Compare January 19, 2017 01:36
@bangert
Copy link
Collaborator

bangert commented Jan 20, 2017

the travis failure here:
SyntaxError: Use of const in strict mode.

is due to using nodejs 0.10. I am not objected to increasing the minimum required node version to 4.x (which apparently is the minimum version having enabled es6 also in strict mode) og 6.9.x even. However others may not have that luxury...

http://stackoverflow.com/questions/22603078/syntaxerror-use-of-const-in-strict-mode

@Eden-Sun
Copy link
Contributor Author

Eden-Sun commented Jan 20, 2017

thats why i switch to circle-ci, only the module eslint need ES6 syntax which is a dev- dependency , so we can develop and run lint under node 4.x+ , and run only npm test under node 0.10.
just like

machine:
  node:
    version: 4

test:
  override:
    - npm run lint && npm run test
    - nvm use 0.10; npm test 

here

@Eden-Sun
Copy link
Contributor Author

Eden-Sun commented Jan 20, 2017

It will not increase the minimum node requirement since we can skip lint on lower version.
The problem is that wether we should do a hint/lint on CI check.
If yes, that is the solution.
If not, it does not matter what hinter / linter to choose.

@calmh
Copy link
Owner

calmh commented Jan 20, 2017

Node 0.10 is simply what was the thing when I created that file, feel free to update as needed. Today I'd probably prefer circle before Travis as well, but either way works for me. Staying with Travis probably creates slightly less work for me, which is preferable from that standpoint.

@bangert
Copy link
Collaborator

bangert commented Jan 24, 2017

Lets increase the node version then...

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

Successfully merging this pull request may close these issues.

3 participants