8000 [#47] Add action to switch between Source/Header by ddscharfe · Pull Request #48 · eclipse-cdt/cdt-lsp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[#47] Add action to switch between Source/Header #48

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
Merged
merged 2 commits into from
May 9, 2023
Merged

[#47] Add action to switch between Source/Header #48

merged 2 commits into from
May 9, 2023

Conversation

ddscharfe
Copy link
Contributor
@ddscharfe ddscharfe commented Apr 25, 2023 •
edited by jonahgraham
Loading
  • add server interface for clangd textDocument/switchSourceHeader extension
  • add command and menu entry for switch action

Note: In the UI the term "toggle" is used instead of switch in order to be consistent with the CDT editor.

Fixes #47

@jonahgraham
Copy link
Member

I added a Fixes #47 to the OP comment - having the [#47] in the subject isn't enough for GitHub to automatically link the PR to the Issue.

@ddscharfe
Copy link
Contributor Author

I added a Fixes #47 to the OP comment - having the [#47] in the subject isn't enough for GitHub to automatically link the PR to the Issue.

Thanks, @jonahgraham

@ddscharfe
Copy link
Contributor Author

Added a second commit to support cdt-lsp editors embedded in MultiPageEditorPart editors.

@ddscharfe ddscharfe force-pushed the issue-47 branch 2 times, most recently from de85b60 to fdf1608 Compare April 26, 2023 13:37
@ghentschke
Copy link
Contributor

LGTM. @ddscharfe could you please stash the commits and provide a test for this stuff?
8000

ruspl-afed
ruspl-afed reviewed Apr 26, 2023
View reviewed changes
@ddscharfe
Copy link
Contributor Author

LGTM. @ddscharfe could you please stash the commits and provide a test for this stuff?

I think you meant squash the commits - did that.

I added tests for the property tester. I did not write tests for the command handler, because it consists mostly of calling (static) methods. If we want to test code like this with unit tests we should use DI (which I think would be a good thing). What I'm not sure about if in the case of the handler it would make sense to create integration tests.
I introduced a dependency to mockito, I hope this is okay. I also had to add dependencies to net.bytebuddy.byte-buddy(-agent) to make mockito work. What I couldn't use is the Mockito Extension, because mockito-junit-jupiter doesn't seem to be in the cdt target platform. Don't know what @jonahgraham thinks about this, maybe we can add it there?

I also added some more JavaDoc as suggested by @ruspl-afed.

@ddscharfe
Copy link
8000
Contributor Author

Seems like the mockito dependency cannot be resolved, I was assuming we are using the cdt target platform?

@ghentschke
Copy link
Contributor

Seems like the mockito dependency cannot be resolved, I was assuming we are using the cdt target platform?

I think this has to be changed since we are now part of cdt.

@ruspl-afed
Copy link
Member

@ddscharfe what if you add the required bundles to the target here ? Then we can add them to the CDT target.

@ddscharfe
Copy link
Contributor Author

@ddscharfe what if you add the required bundles to the target here ? Then we can add them to the CDT target.

Thanks, I overlooked that.

I added the latest orbit repository for mockito, because the orbit repository (which is used for Guava and SnakeYAML) doesn't contain mockito-junit-jupiter.

@ddscharfe
Copy link
Contributor Author

Maven fails with this error:

java.lang.AbstractMethodError: Receiver class org.junit.jupiter.engine.descriptor.MethodExtensionContext does not define or inherit an implementation of the resolved method 'abstract java.util.Optional getTestInstances()' of interface org.junit.jupiter.api.extension.ExtensionContext.

I have no idea what this means, probably not using MockitoExtension would solve this problem. Any ideas?

ruspl-afed
ruspl-afed approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member
@ruspl-afed ruspl-afed left a comment

Choose a reason for hiding this comment

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

LGTM

@ruspl-afed
Copy link
Member

Perhaps it was the mix of mockito versions.
Should we merge it or do you plan more work here @ddscharfe ?
8000

@ddscharfe
Copy link
Contributor Author
ddscharfe commented Apr 27, 2023 •
edited
Loading

Maven fails with this error:

java.lang.AbstractMethodError: Receiver class org.junit.jupiter.engine.descriptor.MethodExtensionContext does not define or inherit an implementation of the resolved method 'abstract java.util.Optional getTestInstances()' of interface org.junit.jupiter.api.extension.ExtensionContext.

I have no idea what this means, probably not using MockitoExtension would solve this problem. Any ideas?

I removed the MockitoExtension and use the R20220531185310 orbit repository for the mockito dependency and now the build succeedes. If anyone knows how to solve the maven error, I'd be happy to hear any suggestions, because I like using @mock. Otherwise we can leave it as it is.
I didn't squash the commits to make this explicit in the history, hope this is okay.

Perhaps it was the mix of mockito versions. Should we merge it or do you plan more work here @ddscharfe ?

I think we can merge it.

@ddscharfe
Copy link
Contributor Author

Forgot to implement LspEditorActiveTesterTest::editorPartWithInnerMatchingEditor properly, fixed it.

@jonahgraham
Copy link
Member

Don't know what @jonahgraham thinks about this, maybe we can add it there?

No problem adding the dependencies - with #51 (thanks @ruspl-afed) it will be easy to check that such new dependencies work well.

@ghentschke
Copy link
Contributor

@jonahgraham I think this PR can be rebased and merged. Could you do that please.

@jonahgraham
Copy link
Member

@jonahgraham I think this PR can be rebased and merged. Could you do that please.

on it

ddscharfe added 2 commits May 9, 2023 09:50
@ddscharfe @jonahgraham
[eclipse-cdt#47] Add action to switch between Source/Header
d147888 
- add server interface for clangd textDocument/switchSourceHeader
extension
- add command and menu entry for switch action
- add orbit repository to target platform (required for Mockito)

Note: In the UI the term "toggle" is used instead of switch
in order to be consistent with the CDT editor.
@ddscharfe @jonahgraham
[eclipse-cdt#47] Add action to switch between Source/Header
9f36ab9 
- don't use MockitoExtension as it causes build problems with maven
@jonahgraham
Copy link
Member

I didn't squash the commits to make this explicit in the history, hope this is okay.

I missed that on what I just force pushed - I am doing another force push now

@jonahgraham jonahgraham merged commit 0f89dd0 into eclipse-cdt:master May 9, 2023
@jonahgraham jonahgraham added this to the 1.0.0 milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruspl-afed ruspl-afed ruspl-afed approved these changes

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

Successfully merging this pull request may close these issues.

Action to Switch between Source/Header
4 participants
@ddscharfe @jonahgraham @ghentschke @ruspl-afed
0