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.

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

Provide fix suggestions for not covered Maven plugin execution in project build lifecycle #1949

Merged
merged 4 commits into from
Jun 24, 2021

Conversation

testforstephen
Copy link
Collaborator

Signed-off-by: Jinbo Wang jinbwan@microsoft.com

This PR requires the side PR at eclipse-jdtls/eclipse.jdt.ls#1770. It's used to mitigate the issues at #177, #566, #935 and #1910.

Here are the main things this PR added.

  • Change the default value of preference "java.configuration.maven.notCoveredPluginExecutionSeverity" to "error", and show error diagnostics to user for awareness.
  • Provide quickfix suggestions for not covered Maven plugin execution in pom.xml.

@testforstephen
Copy link
Collaborator Author

testforstephen commented May 17, 2021

See a sample project https://github.com/antlr/stringtemplate4 to try this PR.
See a demo about the UX.

notCoveredExecution.mov

A follow-up improvement is to "discover new m2e connectors". Eclipse provides such a quick fix when reporting "not covered execution" error in Maven project. Looks like it's based on the catalog https://repo1.maven.org/maven2/.m2e/discovery-catalog/connectors.xml for discovery. Need more evaluation on the cost to support discovering and installing m2e connector in VS Code, will take it as a future improvement.

// @fbricon Could you help review the solution? thanks.

@testforstephen testforstephen marked this pull request as ready for review May 18, 2021 03:32
@testforstephen testforstephen requested a review from fbricon May 18, 2021 03:32
Copy link
Collaborator

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

I guess it will help in some cases, but I'm afraid we're opening a can of worms with the whole m2e connector thing (especially the discovery mechanism).

The best thing is still for maven plugins authors to provide native support for m2e. The doc should probably encourage users to request such integration directly from these projects.

document/_java.notCoveredExecution.md Outdated Show resolved Hide resolved
@testforstephen testforstephen force-pushed the jinbo_severity branch 2 times, most recently from b325d59 to 31a2adc Compare May 19, 2021 07:49
@testforstephen
Copy link
Collaborator Author

Yes, the m2e connectors are a bit too complex and not planned to add them to VS Code. We are currently just providing some suggestions for common workarounds, and also stating in the doc that the best approach would be to make the Maven plugin directly compatible with m2e.

@testforstephen
Copy link
Collaborator Author

Update:

  • The latest commit has changed the default severity level to "warning".
  • The PI didn't break the deployment to maven central.
    Tried deploying a pom with PI <?m2e execute onConfiguration?> to oss.sonatype.org, it worked. And reference the sample dependency in other maven project, no issues were found.

@rgrunber
Copy link
Member

I will look at this today.

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Seems to be working well. Just a few corner cases to consider.

src/pom/pomCodeActionProvider.ts Outdated Show resolved Hide resolved
src/pom/pomCodeActionProvider.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
…ject build lifecycle

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
…he Maven plugins

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge when ready. I would probably squash the commits first.

@testforstephen testforstephen merged commit 7ac2be5 into redhat-developer:master Jun 24, 2021
@testforstephen testforstephen deleted the jinbo_severity branch June 24, 2021 01:19
@testforstephen testforstephen added this to the Mid June 2021 milestone Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants