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

Refactor prototypes to ES6 classes #7849

Closed
kaicataldo opened this issue Jan 4, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@kaicataldo
Copy link
Member

commented Jan 4, 2017

In the spirit of #6407 and #7810 (comment), I was thinking we could update the codebase to use ES6 classes. I'd love to put the "help wanted" label on this as I think it could be a great first contribution for new contributors.

Here's a list of places where these changes could be made (please let me know if I missed any!):

@eslintbot eslintbot added the triage label Jan 4, 2017

@kaicataldo kaicataldo added chore evaluating and removed triage labels Jan 4, 2017

kaicataldo added a commit that referenced this issue Jan 4, 2017

@kaicataldo kaicataldo changed the title Refactor prototypes to ES6 classes Chore: refactor lib/config to ES6 class (refs #7849) Jan 4, 2017

@kaicataldo kaicataldo changed the title Chore: refactor lib/config to ES6 class (refs #7849) Chore: refactor lib/config to ES6 class Jan 4, 2017

@kaicataldo kaicataldo changed the title Chore: refactor lib/config to ES6 class Refactor prototypes to ES6 classes Jan 4, 2017

@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2017

Sorry, got my tabs mixed up 😅

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

I already started the work: 😃

@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2017

Oh wow, I actually hadn't seen your PRs...Great minds, eh?

gyandeeps added a commit that referenced this issue Jan 4, 2017

gyandeeps added a commit that referenced this issue Jan 4, 2017

@gyandeeps gyandeeps added accepted and removed evaluating labels Jan 4, 2017

gyandeeps added a commit that referenced this issue Jan 4, 2017

gyandeeps added a commit that referenced this issue Jan 4, 2017

gyandeeps added a commit that referenced this issue Jan 5, 2017

kaicataldo added a commit that referenced this issue Jan 6, 2017

@qlaire

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

@kaicataldo I've found some more places where changes could be made:

  • lib/cli-engine.js
  • lib/testers/rule-tester.js
  • lib/util/source-code-fixer.js
  • lib/util/source-code.js
  • lib/util/traverser.js
  • tests/lib/config-file.js

Please let me know if I should go ahead and work on those as well.

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@qlaire Thanks for taking the time.
To start, lets go with what we have in the issue (unchecked) because some of the file we have deliberately ignored because they are exposed on the API.
Let us know if you have any question or need any help to get started. Feel free to stop by our chat room for quick response: https://gitter.im/eslint/eslint?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge

@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2017

@qlaire Thanks for contributing! Here's some more info for some context around what @gyandeeps mentioned: #7846 (comment)

@qlaire

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

@gyandeeps @kaicataldo thanks, both!

qlaire added a commit to qlaire/eslint that referenced this issue Jan 10, 2017

qlaire added a commit to qlaire/eslint that referenced this issue Jan 10, 2017

@qlaire

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

@kaicataldo PR in: #7891 😄

qlaire added a commit to qlaire/eslint that referenced this issue Jan 10, 2017

@oksas

This comment has been minimized.

Copy link

commented Jan 11, 2017

Looks like @qlaire's PR #7891 takes care of all of this except lib/util/glob; is there anyone already working on that one? I would love to take a stab at it if not!

@vedatmahir

This comment has been minimized.

Copy link

commented Jan 11, 2017

Hi, @oksas!
I've already started working on that file.

vedatmahir added a commit to vedatmahir/eslint that referenced this issue Jan 11, 2017

@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

Thanks everyone for contributing!

ilyavolodin added a commit that referenced this issue Jan 11, 2017

Chore: update to use ES6 classes (refs #7849) (#7891)
* Chore: update to ES6 classes (refs #7849)

* Chore: refactor ruleFixer to be plain object (refs #7849)

* Chore: make suggested changes (refs #7849)
@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

It looks like we're done with all the ones in the original issue (lib/util/glob.js turned out to be a case where should leave it as is), but would we like to update the issue with any @qlaire's suggestions? I believe lib/cli-engine.js and lib/testers/rule-tester.js are out because they're part of the public API, but how about the other suggestions?

@not-an-aardvark not-an-aardvark added this to InProgress in v4.0.0 Feb 23, 2017

not-an-aardvark added a commit that referenced this issue Mar 11, 2017

not-an-aardvark added a commit that referenced this issue Mar 11, 2017

@kaicataldo

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2017

I believe this issue has been completed. If there are any other instances where we can convert to ES6 classes, please feel free to make a new issue/PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.