Skip to content

Add eslint rules to project as suggested in #30#34

Open
nabrahamson wants to merge 1 commit intosavoirfairelinux:masterfrom
nabrahamson:enable-eslint
Open

Add eslint rules to project as suggested in #30#34
nabrahamson wants to merge 1 commit intosavoirfairelinux:masterfrom
nabrahamson:enable-eslint

Conversation

@nabrahamson
Copy link
Collaborator

Fixes #30 by @ventilooo

Changes proposed in this pull request:

  • Added the .eslintrc.json configuration file
  • Modified the CONTRIBUTING.md file with instructions for eslint
  • linted src/ringme.js so it conforms to all coding standards with
    exception to the modules.export line
  • Added a note to CONTRIBUTING.md suggesting that eslint be
    installed as a global npm package

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

  1. Follow the instructions in CONTRIBUTING.md to configure eslint
    on your development machine
  2. Run eslint with the command
eslint **/*.js
  1. Verify that ./src/ringme.js has three linting errors
  77:7  error  Unexpected console statement  no-console
  284:1  error  Unexpected 'todo' comment     no-warning-comments
  285:1  error  'module' is not defined       no-undef

Additional notes

Based on the maintainer's preference for this project, I recommend the following further tasks

  • In package.json, add a lint command
  • Disable the no-console rule in .eslintrc.json
  • Temporarily disable the no-warning-comments rule until project is out of alpha

* Added the .eslintrc.json configuration file
* Modified the CONTRIBUTING.md file with instructions for eslint
* linted src/ringme.js so it conforms to all coding standards with
exception to the modules.export line
@ssirois
Copy link
Contributor

ssirois commented Oct 6, 2017

Welcome to ringme.js @nabrahamson and thank you for this contribution.

There was already a PR (#32) that adds ESLint to the project as your PR does. Since it arrived first (a couple hours before this one), I think I would be fair to merge #32 but there is valuable stuff in your PR that is not present in #32.

After merging #32, I shall take a look at your PR and take what will enhance the solution. I see that there is only one commit on this PR so it will be hard for me to cherry pick the documentation, for example.

Let's keep this PR open. We shall then take a look at the diffs against the trunk that will contain ESLint and a conversation of enhancements and bonuses coming from stuff in here could then start here.

Is that OK with you?

@ssirois
Copy link
Contributor

ssirois commented Oct 7, 2017

@nabrahamson it would be much appreciated if you could check this branch against an updated version of the master branch and propose changes.

If you think some ESLint configurations could be tweaked a little and check out the CONTRIBUTING.md file to improve it with documentation you have provided here, it would be awesome.

@ventilooo
Copy link
Contributor

ventilooo commented Oct 16, 2017

@nabrahamson , could you fix conflict please 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants