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 js2-declare-variable #219

Closed
wants to merge 15 commits into from
Closed

Add js2-declare-variable #219

wants to merge 15 commits into from

Conversation

jacksonrayhamilton
Copy link
Contributor

I have a little present for you: A new utility function, js2-declare-variable!

js2-declare-variable makes it easy to extend var statements. Bound to C-c C-v, just enter the name of a variable (and optionally its initial value) and js2-declare-variable will append the declaration to the first var statement in the scope enclosing point.

If there isn't already a var statement in the enclosing scope, one will be created.

If there is a "use strict" statement or comments at the top of the enclosing scope, the var statement will be inserted after them.

In implementing this, I discovered a bug in js2-node-get-enclosing-scope where a node without a parent would trigger an error, because js2-node-parent could not operate on nil. The new fuction requires js2-node-get-enclosing-scope to behave correctly, so I fixed it.

I also noticed #218 when writing the js2-declare-variable-comment-* tests, so I simply added an extra newline in those tests to prevent that particular error. I would rather they not be there, so I marked them with TODOs until that bug is fixed. (Update: e054673 and efdc5f6 fixed that.)

I have signed the copyright forms for Emacs and they have been approved; I guess that means I can legally contribute to this repository as well?

I hope you like this feature and thanks for your review.

js2-declare-variable makes it easy to extend var statements.  Bound to
C-c C-v, just enter the name of a variable (and optionally its initial
value) and js2-declare-variable will append the declaration to the first
var statement in the scope enclosing point.

If there isn't already a var statement in the enclosing scope, one will
be created.

If there is a "use strict" statement or comments at the top of the
enclosing scope, the var statement will be inserted after them.
Cover some additional cases where non-var-statement scopes would be
candidates for var statements in js2-declare-variable.
@jacksonrayhamilton
Copy link
Contributor Author

This PR now depends on #220.

@dgutov
Copy link
Collaborator

dgutov commented Feb 28, 2015

I can see quite a bit of effort went into this, but are you sure js2-mode is the best place for it, as opposed to js2-refactor? It implements a set of editing commands already, and this one might be a welcome addition.

I have signed the copyright forms for Emacs and they have been approved; I guess that means I can legally contribute to this repository as well?

It does. Magnar also signed it, so it probably means we could add js2-refactor to GNU ELPA without too much effort, in case that's your primary interest in contributing this command here.

@jacksonrayhamilton
Copy link
Contributor Author

Seeing as #159 is pending I assumed this would be an okay place to put it.

I'd have to rewrite the tests, but if you think it belongs there, I can suggest it to the maintainer. We don't necessarily need to get the package on ELPA, do we? I can just as easily use MELPA.

@dgutov
Copy link
Collaborator

dgutov commented Feb 28, 2015

Seeing as #159 is pending I assumed this would be an okay place to put it.

That pull request is different: it's navigation, not editing. I don't think we actually have any editing commands here. And we add one, the other ones must follow, and then we'll start duplicating js2-refactor.

I'd have to rewrite the tests, but if you think it belongs there, I can suggest it to the maintainer.

Let's start with asking him. Maybe you won't have to rewrite the tests that much.

We don't necessarily need to get the package on ELPA, do we?

Sure, it's up to you. ELPA just means a wider audience.

@jacksonrayhamilton
Copy link
Contributor Author

I've continued development of this feature in js2-refactor and the author seems interested. Thanks for pointing me in the right direction.

@jacksonrayhamilton jacksonrayhamilton deleted the declare-variable branch April 6, 2015 22:52
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.

None yet

2 participants