-
Notifications
You must be signed in to change notification settings - Fork 13
fix compatibility with IDEA 2016.1 and above #186
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
base: master
Are you sure you want to change the base?
Conversation
|
@TomDevs, thanks for updating the plugin to work with the latest versions of WebStorm. Will try it out, downloading from yours (https://github.com/TomDevs/needsmoredojo/tree/master/dist), since it looks like @cefolger does not support the repo anymore. |
|
@TomDevs Many thanks for the update. |
sindilevich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also line#47 be changed to Collections.addAll(parameters, function.getParameterVariables());?
| } | ||
|
|
||
| if(imports.getChildren().length == 0) | ||
| if(imports.getExpressions().length == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of using getExpressions() over getChildren() here? #181 does not changed that, so am asking for general knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem that when adding an import (Ctrl+Shift+O, 2), the import was added in the wrong location, usually outside of the JavaScript import array. Using getExpressions() fixed this issue.
| int sourceIndex = PsiUtil.getIndexInParent(defines[0]); | ||
| int destinationIndex = PsiUtil.getIndexInParent(defines[1]); | ||
| JSParameter[] parameterList = items.getFunction().getParameters(); | ||
| JSParameter[] parameterList = items.getFunction().getParameterVariables(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also src/com/chrisfolger/needsmoredojo/core/amd/define/NearestAMDImportLocator.java still use the old getFunction().getParameters() in line 60 and line 123.
And src/com/chrisfolger/needsmoredojo/core/amd/importing/ImportUpdater.java still use the old getFunction().getParameters() in line 65 and line 74.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a lot of cases getParameters() and getParameterVariables() can be used to do the same things, so it shouldn't really matter which method is used; I changed the ones necessary to fix the test cases and get the plugin to a working state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomDevs, thank you fro your reply. You may be right, replacing the minimum possible, but there was a problem reported here: #181 (comment) that was handled by replacing getFunction().getParameters() with getFunction().getParameterVariables(), as per 427bc53. That's why I wanted to point out these places to you to look closer at.
| <id>com.chrisfolger.needsmoredojo</id> | ||
| <name>Needs More Dojo</name> | ||
| <version>0.7</version> | ||
| <version>0.8.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#181 suggests moving the file to resources/META-INF/plugin.xml and updating resources/META-INF/MANIFEST.MF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file location particularly matters, but if this becomes a problem I'll happily move it.
Uh oh!
There was an error while loading. Please reload this page.