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

chore: add lint step #77

Merged
merged 6 commits into from
Oct 24, 2018
Merged

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Oct 9, 2018

Description

When doing my recent PR, I noticed a lot of small things by eye that would be caught by a linter, so here you go.

If you don't want a lint step, im happy to abandon this. Its a "while i was in there" PR.

This is Incomplete

If you want this change, you need to decide which config/guide you'd like to follow (e.g. google, airbnb).

It currently uses eslint:recommended which is very lenient. I would suggest the google config with jsdoc requirement turned off.

I do realise you use closure compiler, too, but wasn't sure if that already [tries] to pick up such issues (I haven't used CC in a while).

New Feature

  • If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
  • Run unit tests to ensure all existing tests are still passing
  • Add new passing unit tests to cover the code introduced by your PR
  • Update the readme
  • Add some steps so we can test your cool new feature!
  • The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

$ npm run lint

@43081j 43081j force-pushed the lint branch 2 times, most recently from b0ed366 to e60fce4 Compare October 9, 2018 20:17
@frederickfogerty
Copy link
Contributor

Hi there @43081j. Thank you for the PR, and thank you for picking up that we need a linter. On other projects we've used prettier, so I'd be keen to use prettier-eslint or similar here. I've added this to our backlog to fix for both this library and Drift. If this PR was updated to use prettier, that would be awesome and I'd love to merge it, but for now I'm going to reject it.

@43081j
Copy link
Contributor Author

43081j commented Oct 15, 2018

From what I remember, prettier-eslint is only a means of formatting your sources before they are passed through eslint, rather than a ruleset its self.

That means you'd still need a good eslint config in place to account for non-stylistic issues such as const vs let, shadowing, reassignment of a const, etc.

If you have a preference of what eslint config to use (if any, maybe just the recommended one with some extras enabled), i'd be happy to update this so it uses that and prettier-eslint.

@frederickfogerty
Copy link
Contributor

Completely agree, a good ruleset is needed.

I'd like to avoid stylistic rules as much as possible and just have the non-stylistic rules turned on.

It's been a while since I've used eslint, do you have any configurations that you particularly like? It seems that eslint-config-airbnb is the most used, so if you have no preference, let's go with that.

I'll take you up on the offer to update this PR with prettier-eslint. Thanks @43081j!

@43081j
Copy link
Contributor Author

43081j commented Oct 15, 2018

I do like google's ruleset but combined with eslint:recommended.

We would need eslint-config-prettier to disable the stylistic rules it'll turn on, too.

I'll give it a go and see how it goes.

@43081j
Copy link
Contributor Author

43081j commented Oct 15, 2018

@frederickfogerty here's a first try.

it uses:

  • eslint-config-google
  • eslint-config-prettier (to disable stylistic rules that conflict with prettier)
  • eslint:recommended (common recommended stuff)

google requires jsdoc so i changed it from error to warn and added jsdoc to the existing source.

I didn't have to add all the jsdoc but figured i may as well while im in there. if any of it sounds wrong, feel free to comment what it should be.

we didn't need prettier-eslint as we can still do npm run format just fine, and our rules don't conflict with it.

Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

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

LGTM!

* master:
  chore(deps): update dependency cssnano-preset-advanced to v4.0.5 (strawdynamics#98)
  chore(deps): update dependency cssnano to v4.1.7 (strawdynamics#97)
  chore(deps): update dependency postcss-cli to v6 (strawdynamics#96)
  chore(deps): update dependency karma to v3 (strawdynamics#95)
  chore(deps): update dependency closure-webpack-plugin to v1 (strawdynamics#81)
  chore(deps): update dependency babel-eslint to v10 (strawdynamics#94)
  chore(deps): update dependency webpack-cli to v3.1.2 (strawdynamics#93)
  chore(deps): update dependency webpack to v4.22.0 (strawdynamics#92)
  chore(deps): update dependency prettier to v1.14.3 (strawdynamics#91)
  chore(deps): update dependency jasmine-core to v3.2.1 (strawdynamics#90)
  chore(deps): update dependency cssnano to v4.1.5 (strawdynamics#89)
  chore(deps): update dependency chai to v4.2.0 (strawdynamics#88)
  chore(deps): update babel monorepo (strawdynamics#87)
  chore(deps): update dependency yargs to v12.0.2 (strawdynamics#86)
  chore(deps): update dependency cssnano-preset-advanced to v4.0.3 (strawdynamics#84)
  chore(deps): update dependency babel-loader to v8.0.4 (strawdynamics#83)
  chore(deps): pin dependencies (strawdynamics#80)
  chore(travis): only run build check on master and release branches (strawdynamics#82)
  chore(renovate): add Renovate (strawdynamics#78)

# Conflicts:
#	package-lock.json
#	package.json
* commit '67413e81639227f354852bdd7b8e2e3495e8e354':
  chore(deps): update dependency karma to v3.1.1 (strawdynamics#99)

# Conflicts:
#	package-lock.json
@frederickfogerty frederickfogerty merged commit 7e195d6 into strawdynamics:master Oct 24, 2018
@43081j 43081j deleted the lint branch October 25, 2018 00:05
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