Skip to content

[READY] Update JDT to 0.40.0 #1266

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

Merged
merged 1 commit into from
Jun 23, 2019
Merged

[READY] Update JDT to 0.40.0 #1266

merged 1 commit into from
Jun 23, 2019

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Jun 15, 2019

A few significant changes:

  • JDT doesn't send overlapping code actions or nonsense like "delete+insert"
  • JDT loves getters and setters, so those fixits are now everywhere.
  • JDT loves to organize imports, so those fixits are now everywhere.

This change is Reviewable

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Thanks so much !

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/tests/java/get_completions_test.py, line 664 at r1 (raw file):

          CompletionEntryMatcher( 'InputStreamReader', None, {
            'kind': 'Class',
            'menu_text': 'InputStreamReader - jdk.internal.jline.internal',

This seems... odd. Why is it returning suggestions for jdk.internal. That seems like a jdk.ls bug to me!


ycmd/tests/java/subcommands_test.py, line 1003 at r1 (raw file):

Quoted 4 lines of code…
        # Again, this produces quite a lot of fussy little changes (that
        # actually lead to broken code, but we can't really help that), and
        # having them in this test would just be brittle without proving
        # anything about our code

not sure we need the whole of this comment.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/java/get_completions_test.py, line 664 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This seems... odd. Why is it returning suggestions for jdk.internal. That seems like a jdk.ls bug to me!

How would I know why it returns jdk.internal.

Check the logs... it's even more nuts!

'detailed_info': 'InitialServerRequestDispatcher - com.sun.corba.se.spi.protocol\n\n',

And not only that, but it returns just java.io and that Sun thing... This is exactly the behaviour I saw on my laptop on 0.38.0.

I have no idea how to solve this, except by adding com.sun.cobra blah blah blah... but that's fixing the consequences, not the source of the problem.


ycmd/tests/java/subcommands_test.py, line 1003 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…
        # Again, this produces quite a lot of fussy little changes (that
        # actually lead to broken code, but we can't really help that), and
        # having them in this test would just be brittle without proving
        # anything about our code

not sure we need the whole of this comment.

Copy/paste from some other place. Removed.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/java/get_completions_test.py, line 664 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

How would I know why it returns jdk.internal.

Check the logs... it's even more nuts!

'detailed_info': 'InitialServerRequestDispatcher - com.sun.corba.se.spi.protocol\n\n',

And not only that, but it returns just java.io and that Sun thing... This is exactly the behaviour I saw on my laptop on 0.38.0.

I have no idea how to solve this, except by adding com.sun.cobra blah blah blah... but that's fixing the consequences, not the source of the problem.

shrug, probably not our bug whatever it is.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 LGTMs obtained

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/tests/java/get_completions_test.py, line 664 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

shrug, probably not our bug whatever it is.

But how do I make tests pass?

I'm guessing the difference in response could be because of slightly different jdk versions. Should I just try to satisfy Azure?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/java/get_completions_test.py, line 664 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

But how do I make tests pass?

I'm guessing the difference in response could be because of slightly different jdk versions. Should I just try to satisfy Azure?

Okay, my stupid! Reverted to has_item(). This test should pass now.

@bstaletic bstaletic force-pushed the jdt40 branch 3 times, most recently from 10988f4 to a8d262b Compare June 16, 2019 06:28
@codecov

This comment has been minimized.

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #1266 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1266     +/-   ##
=========================================
+ Coverage   96.94%   97.15%   +0.2%     
=========================================
  Files          96       96             
  Lines        7375     7383      +8     
  Branches      153      153             
=========================================
+ Hits         7150     7173     +23     
+ Misses        184      169     -15     
  Partials       41       41

@bstaletic bstaletic force-pushed the jdt40 branch 3 times, most recently from f6221ec to b61ebc5 Compare June 16, 2019 22:15
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Fixed a flake8 bug, so this should be ready now.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

@puremourning
Copy link
Member

ok let's rebase this one too now, shall we ?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Rebased, but the issue with this one is that jdt on python2 on Windows doesn't seem to shut down, so the tests hang indefinitely.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

@bstaletic bstaletic force-pushed the jdt40 branch 3 times, most recently from 904abc1 to 2430209 Compare June 22, 2019 22:14
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Finally green!

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/test_utils.py, line 453 at r3 (raw file):

def PollForMessages( app, request_data, timeout = 60 ):

I changed this default timeout to 60 because Poll_Diagnostics_ProjectWide_Eclipse_test was quite flaky.

Is it okay to change the default or would it be better to change this in one java test that is actually flaky?

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/tests/test_utils.py, line 453 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I changed this default timeout to 60 because Poll_Diagnostics_ProjectWide_Eclipse_test was quite flaky.

Is it okay to change the default or would it be better to change this in one java test that is actually flaky?

Default is fine

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

@puremourning puremourning added the Ship It! Manual override to merge a PR by maintainer label Jun 23, 2019
@mergify mergify bot merged commit 2ebaca9 into ycm-core:master Jun 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2019

Thanks for sending a PR!

bstaletic added a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 3, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic added a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic added a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic added a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic added a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 5, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants