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.

Sign up for GitHub

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

Jump to bottom

Add unimplemented methods code action. #322

Merged
merged 1 commit into from
Aug 8, 2017
Merged

Add unimplemented methods code action. #322

merged 1 commit into from
Aug 8, 2017

Conversation

yaohaizh
Copy link
Contributor

@yaohaizh yaohaizh commented Aug 4, 2017 •
edited

fbricon
fbricon requested changes Aug 4, 2017
View reviewed changes
...src/org/eclipse/jdt/ls/core/internal/corrections/proposals/LocalCorrectionsSubProcessor.java Outdated
private static final String ADD_FIELD_QUALIFICATION_ID = "org.eclipse.jdt.ui.correction.qualifyField"; //$NON-NLS-1$
private static final String ADD_STATIC_ACCESS_ID = "org.eclipse.jdt.ui.correction.changeToStatic"; //$NON-NLS-1$
private static final String REMOVE_UNNECESSARY_NLS_TAG_ID = "org.eclipse.jdt.ui.correction.removeNlsTag"; //$NON-NLS-1$

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a whole lot of commented code, bringing this contribution way past the 1000L threshold that requires us to open a CQ.

Can you get rid of the commented stuff?

...src/org/eclipse/jdt/ls/core/internal/corrections/proposals/LocalCorrectionsSubProcessor.java Outdated
private static final String ADD_NON_NLS_ID = "org.eclipse.jdt.ui.correction.addNonNLS"; //$NON-NLS-1$
private static final String ADD_FIELD_QUALIFICATION_ID = "org.eclipse.jdt.ui.correction.qualifyField"; //$NON-NLS-1$
private static final String ADD_STATIC_ACCESS_ID = "org.eclipse.jdt.ui.correction.changeToStatic"; //$NON-NLS-1$
private static final String REMOVE_UNNECESSARY_NLS_TAG_ID = "org.eclipse.jdt.ui.correction.removeNlsTag"; //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the org.eclipse.jdt.ui prefixes to org.eclipse.jdt.ls

@yaohaizh
Add unimplemented methods code action.
a718bb6 
Signed-off-by: Yaohai Zheng <yaozheng@microsoft.com>
@yaohaizh
Copy link
Contributor Author

yaohaizh commented Aug 7, 2017 •
edited

Updated the changes

@aeschli
Copy link
Contributor

aeschli commented Aug 7, 2017

@yaohaizh Great work!
Can you bring over the JDT tests as well?
https://github.com/eclipse/eclipse.jdt.ui/blob/master/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/LocalCorrectionsQuickFixTest.java

Search for testUnimplementedMethods*, there's like 23 tests that we have in JDT.
The trick to avoid too much typing and reformatting is to just copy the code that creates the test, expect no results, then run the tests (they will fail). Check out the failure... It will contain a code snippet with some code you can copy... Of course only do that of the actual results makes sense.

@yaohaizh
Copy link
Contributor Author

yaohaizh commented Aug 7, 2017

@fbricon @aeschli If I copies these test cases, do them count on the whole loc?

@fbricon
Copy link
Contributor

fbricon commented Aug 7, 2017

@yaohaizh yes, that would count as part of the total LOC contribution. Which sends us back into CQ territory :-/

We can probably merge this one PR first and add more tests later

@yaohaizh
Copy link
Contributor Author

yaohaizh commented Aug 8, 2017 •
edited

@fbricon Changes were pushed based on code review. We can merge this PR first if everything is okay. I'll merge the test cases later in another PR.

@fbricon fbricon mentioned this pull request Aug 8, 2017
Closed
@fbricon
Copy link
Contributor

fbricon commented Aug 8, 2017

PR works well but I found that indentation issue in vscode. I'm not sure whether we should try to infer the expected file indentation at the server level (overriding the project settings) and send that to the client (exactly what vscode does when inserting text edits after completing code).

Anyways, I'll merge this as-is, since this is the current expected behaviour.

@fbricon fbricon merged commit cd8de37 into eclipse-jdtls:master Aug 8, 2017
@yaohaizh yaohaizh deleted the yaohai_gs branch October 13, 2017 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon fbricon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yaohaizh @aeschli @fbricon