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

Start using linting rules from ESLint #96

Open
umaar opened this issue Oct 27, 2017 · 4 comments
Open

Start using linting rules from ESLint #96

umaar opened this issue Oct 27, 2017 · 4 comments

Comments

@umaar
Copy link
Member

umaar commented Oct 27, 2017

Enhancement

There are lots of useful ESLint rules out there, and it'd be nice to explore how to use ESLint rules with TypeScript. I see there is this project: https://github.com/eslint/typescript-eslint-parser which could help. Here are some rules which I think are sensible: https://github.com/sindresorhus/eslint-config-xo/blob/891eb4c9bd1e33af8a12fd6f64d0629c08b88785/index.js#L16-L274

Context: dojo/routing#110 (comment)

Also see: https://github.com/nzakas/eslint-plugin-typescript & https://github.com/buzinas/tslint-eslint-rules

@umaar umaar mentioned this issue Oct 27, 2017
3 tasks
@dylans dylans added this to the 2017.11 milestone Oct 27, 2017
@kitsonk kitsonk added the rc label Nov 6, 2017
@kitsonk kitsonk modified the milestones: 2017.11, rc.1 Nov 6, 2017
@kitsonk
Copy link
Member

kitsonk commented Nov 6, 2017

It would be more than just looking at the rules. We would need to:

  • create a POC using eslint
  • integrate it into grunt-dojo2
  • roll out to all the packages
  • make appropriate changes to cli-create-app
  • make appropriate changes to cli-build-webpack/cli-build-next
  • update all examples and tutorials

Since we have linting, to a degree already, and this is a non-trivial change, I think this effectively becomes a "nice to have" for the RC, scheduling as such.

@umaar
Copy link
Member Author

umaar commented Nov 7, 2017

Just to put a related resource into this ticket: there is a tslint-xo package which normally comes with a set of sensible defaults, and avoids the hassle of development teams having to configure rules one by one. It would be a:

{
	"extends": "tslint-xo"
}

In the tslint.json file for projects.

@kitsonk
Copy link
Member

kitsonk commented Nov 8, 2017

We have a fairly opinionated sense of linting already... While I have respect for other parts of the community, I would not want to tie something as personal and subjective to another project where they make decisions about our code and style guides. We could consider to adopt it, but I would rather iterate on our already established linting and style guide. We have 20 something packages and taking a churn to refactor that...

Also I would much rather us pursue dojo/meta#206. There are only a couple items that would contravene our current style guide and only one I really object to, which is an issue that is open and maybe resolved.

I guess I am struggling to understand why we want to establish standards for things we have already established standards for.

@umaar
Copy link
Member Author

umaar commented Nov 8, 2017

I only suggest it because I feel we have a large number of useful rules either turned off, or not included at all from the tslint.json. Also during code review, a number of things came up which were not caught automatically, things that I'd expect a linter to warn me about in my editor & on Travis (but even better is for prettier to fix it automatically as you say).

@kitsonk kitsonk added rc2 and removed rc labels Dec 11, 2017
@kitsonk kitsonk modified the milestones: rc.1, rc.2 Dec 11, 2017
@dylans dylans removed this from the rc.2 milestone Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants