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

Move some eslint methods to SourceCode #3516

Closed
nzakas opened this Issue Aug 25, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@nzakas
Member

nzakas commented Aug 25, 2015

Now that SourceCode is merged, I'd like to move more of the AST-specific functionality to it. The eslint object right now has a mix of methods such as getComments() that apply only to the AST and really should never have been on the object.

My goal is to move these methods to SourceCode and then have the methods on eslint just delegate to SourceCode. We can't really remove those methods since others might be relying on them, but we can certainly refactor so that the code doesn't live inside of the eslint object.

@nzakas nzakas self-assigned this Aug 25, 2015

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Aug 25, 2015

Member

Right now they are actually getting delegated from ast-utils module. so you are going to move it to SourceCodemodule?

Member

gyandeeps commented Aug 25, 2015

Right now they are actually getting delegated from ast-utils module. so you are going to move it to SourceCodemodule?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Aug 25, 2015

Member

Oh okay, somehow I missed that. I think they should all be together in SourceCode, and there are other methods on eslint that should not be there as well:

  • getSource()
  • getNodeByRangeIndex()
  • getSourceLines()
  • getAllComments()
Member

nzakas commented Aug 25, 2015

Oh okay, somehow I missed that. I think they should all be together in SourceCode, and there are other methods on eslint that should not be there as well:

  • getSource()
  • getNodeByRangeIndex()
  • getSourceLines()
  • getAllComments()
@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Aug 25, 2015

Member

The idea of ast-utils was to have common methods which rely on ast but dont need context.

Member

gyandeeps commented Aug 25, 2015

The idea of ast-utils was to have common methods which rely on ast but dont need context.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Aug 25, 2015

Member

Yup, I remember. I don't think that changes anything.

Member

nzakas commented Aug 25, 2015

Yup, I remember. I don't think that changes anything.

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Aug 25, 2015

Member

What about findJSDocComment? Right now its in ast-utils.

Member

gyandeeps commented Aug 25, 2015

What about findJSDocComment? Right now its in ast-utils.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Aug 25, 2015

Member

That too. It occurred to me over the weekend that the role of ast-utils is really better encapsulated in SourceCode. That way, there's just one object to deal with regardless of what you're trying to do. And then we can start encouraging people to do context.getSourceCode() and work on that so we can keep the number of methods on context where it is right now (already getting a bit too big).

I'll do a first pass with a couple methods and send a PR to show you.

Member

nzakas commented Aug 25, 2015

That too. It occurred to me over the weekend that the role of ast-utils is really better encapsulated in SourceCode. That way, there's just one object to deal with regardless of what you're trying to do. And then we can start encouraging people to do context.getSourceCode() and work on that so we can keep the number of methods on context where it is right now (already getting a bit too big).

I'll do a first pass with a couple methods and send a PR to show you.

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Aug 25, 2015

Member

Sounds good.

Member

gyandeeps commented Aug 25, 2015

Sounds good.

nzakas added a commit that referenced this issue Aug 25, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Aug 25, 2015

Member

By the way, I think the is*() methods will still be on ast-utils.

Member

nzakas commented Aug 25, 2015

By the way, I think the is*() methods will still be on ast-utils.

nzakas added a commit that referenced this issue Aug 25, 2015

@nzakas nzakas closed this in b070f1a Aug 27, 2015

nzakas added a commit that referenced this issue Aug 27, 2015

Merge pull request #3517 from eslint/issue3516
Update: Move methods to SourceCode (fixes #3516)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

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