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

Computing hints based on custom preferences #6760

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Computing hints based on custom preferences #6760

merged 1 commit into from
Jan 17, 2024

Conversation

Achal1607
Copy link
Contributor

@Achal1607 Achal1607 commented Nov 28, 2023 •
edited
Loading

Currently hints are computed on the basis of global default preferences in lsp server. So, added an option to compute hints on the basis of custom hint preferences file if available.

Refer to #javavscode-9

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests hints labels Nov 29, 2023
@mbien mbien added this to the NB21 milestone Nov 29, 2023
@apache apache locked and limited conversation to collaborators Nov 29, 2023
@apache apache unlocked this conversation Nov 29, 2023
@mbien
Copy link
Member

mbien commented Nov 29, 2023 •
edited
Loading

please ignore the lock/unlock. This is our trick to force a new workflow run which takes new labels into account for the jobs. Since you can't label things, someone else has to do that :) (not sure if VSCode tests should run there too, a reviewer will know)

@Achal1607 Achal1607 requested a review from sdedic December 1, 2023 13:01
@Achal1607
Copy link
Contributor Author

Now tests should pass hopefully. There was a missing dependency.

lahodaj
lahodaj requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Some comments inline.

Not sure if you tried to put the augmented server to an enhanced extension to see how it work, but I would suggest to do it, to be sure all parts work together. (Several paths to the configuration seem suspicious here to me.)

ide/api.lsp/src/org/netbeans/spi/lsp/ErrorProvider.java Show resolved Hide resolved
ide/api.lsp/src/org/netbeans/spi/lsp/ErrorProvider.java Outdated Show resolved Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/infrastructure/JavaErrorProvider.java Outdated Show resolved Hide resolved
.../java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/WorkspaceServiceImpl.java Outdated Show resolved Hide resolved
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java Outdated Show resolved Hide resolved
...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java Outdated Show resolved Hide resolved
...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java Outdated Show resolved Hide resolved
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java Show resolved Hide resolved
...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java Outdated Show resolved Hide resolved
@Achal1607 Achal1607 force-pushed the javavscode-9 branch 3 times, most recently from 2361ecd to 9367bd3 Compare December 14, 2023 13:06
@Achal1607
Copy link
Contributor Author

I have updated the code according to the comments provided by @lahodaj and @sdedic. I have pushed the updated code , can someone please review the code again?

@Achal1607 Achal1607 requested a review from sdedic January 12, 2024 07:21
@Achal1607
added support for reading custom hints preferences in lsp
ab0e881 
Signed-off-by: Achal Talati <achal.talati@oracle.com>
@neilcsmith-net neilcsmith-net modified the milestones: NB21, NB22 Jan 16, 2024
lahodaj
lahodaj approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@lahodaj lahodaj merged commit 5a45933 into apache:master Jan 17, 2024
36 checks passed
@mbien
Copy link
Member

mbien commented Jan 17, 2024

this seems to be breaking the VSCode extension build as you can see here: https://github.com/apache/netbeans/actions/runs/7555048750

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jan 17, 2024 •
edited
Loading

Looks like this was merged without the right tests being run on it - labelling failure I guess?! Suggest revert and reapply to stabilise master and review based on what it's failing on (missing module dependency)?

mbien
mbien reviewed Jan 17, 2024
View reviewed changes
java/java.hints/nbproject/project.xml
Comment on lines +168 to +175
<dependency>
<code-name-base>org.netbeans.modules.editor.tools.storage</code-name-base>
<build-prerequisite/>
<compile-dependency/>
<run-dependency>
<specification-version>1.31</specification-version>
</run-dependency>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

this looks like the cause for the build failure:

BUILD FAILED
/home/runner/work/netbeans/netbeans/java/java.lsp.server/build.xml:32: The following error occurred while executing this line:
/home/runner/work/netbeans/netbeans/nbbuild/netbeans/harness/suite.xml:163: The following error occurred while executing this line:
/home/runner/work/netbeans/netbeans/nbbuild/netbeans/harness/build.xml:150: Module org.netbeans.modules.editor.tools.storage excluded from the target platform; the path is: [org.netbeans.modules.nbcode.integration, org.netbeans.modules.java.lsp.server, org.netbeans.modules.java.hints]

Copy link
Contributor Author

@Achal1607 Achal1607 Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, this should be removed from diasabled modules list of vscode extension?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. cc @sdedic @dbalek

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried removing org.netbeans.modules.editor.tools.storage from disabled modules list and it is working fine. I can update the branch and push the code if this PR is reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for this. I think we've missed the "LSP" label in this PR. I've opened:
#6966
What do you think?

@Achal1607 Achal1607 mentioned this pull request Feb 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbien mbien mbien left review comments

@lahodaj lahodaj lahodaj approved these changes

@sdedic sdedic sdedic approved these changes

Assignees
No one assigned
Labels
hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests
Projects
None yet
Milestone
NB22
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Achal1607 @mbien @neilcsmith-net @sdedic @lahodaj