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

Add onScopeIdentifierName callback #38

Closed
wants to merge 4 commits into from

Conversation

dapetcu21
Copy link
Contributor

Hi. I'm working on a Lua autocomplete package for Atom and I figured I would use luaparse.

In my use case, I need to know which identifier is local to which scope ahead of time, since the parsing might error out at the user's cursor as he's editing. That's why I added this callback, which used together with onCreateScope and onDestroyScope gives me a complete idea about the locality of identifiers without having to wait for the node to fully close (for example, a for i = 1, 10 statement would only tell me about i when the block is closed).

@fstirlitz
Copy link
Owner

Looks fine. onScopeIdentifierName is a bit stupid name, though. And I'd squash the three commits which add this callback. (Maybe even squash all four.)

@dapetcu21
Copy link
Contributor Author

So, do you have any suggestion of a name? Maybe onScopeIdentifier?

@fstirlitz
Copy link
Owner

Maybe onDeclareIdentifier (to match onCreateScope/onDestroyScope) or onLocalDeclaration.

@fstirlitz
Copy link
Owner

Also, it would be good to add a testcase; there's already one for the other callbacks in test/runner.js.

@fstirlitz
Copy link
Owner

I went ahead and squashed your commits into two, renaming the callback to onLocalDeclaration along the way (commits ab478b9 and 8a24b63). I've also added a testcase (c82cf9f). Consider it merged.

@fstirlitz fstirlitz closed this Feb 9, 2017
@dapetcu21
Copy link
Contributor Author

Oh. Sorry for not answering. I completely forgot about this PR and went on with my life. Thanks! 😄

@fstirlitz fstirlitz added the enhancement Request for functionality covering an entirely new use case label Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for functionality covering an entirely new use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants