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

Add Managed Version Info on hover #238 #298

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

vrubezhny
Copy link
Contributor

Signed-off-by: Victor Rubezhny [email protected]

@angelozerr
Copy link
Contributor

Just a simple comment when you create a PR. I think it should be nice to add a little screenshot with th eresult of PR (or a little demo). It will be easier to understand what the PR provides and for history it is nice too.

We do that for LemMinX and I find it is nice for the reviewer.

It is just a suggestion that I do.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

Unless I missed something, I believe some extra informations are confusing and should either be fixed or removed.

Comment on lines 98 to 101
assertTrue(value.contains("The managed version is"));
assertTrue(value.contains("2.22.2"));
assertTrue(value.contains("The artifact is managed in"));
assertTrue(value.contains("org.apache.maven.plugins:maven-surefire-plugin:2.22.2"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be added in when the version is the one that is defined in the pom. Managed version are only useful when no version is explicitly set (usually in the "child pom")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we still have to show the version and the location links on a pom where the version is clearly defined.

That was actually a problem as the method I've used for finding version in such case was returning a wrong version and link. So, I'd prefer this test case to also exist for parent pom.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we still have to show the version and the location links on a pom where the version is clearly defined.

we already have that I believe. Ctrl+Click can be used to get the link to the artifact.

assertTrue(value.contains("The managed version is"));
assertTrue(value.contains("2.22.2"));
assertTrue(value.contains("The artifact is managed in"));
assertTrue(value.contains("org.apache.maven.plugins:maven-surefire-plugin:2.22.2"));
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 not correct. The artifact version is managed in pom-dependencyManagement-parent.

Copy link
Contributor Author

@vrubezhny vrubezhny Sep 14, 2022

Choose a reason for hiding this comment

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

Now a version from the parent pom and location of parent pom where the version is defined is shown in Hover.

If no version is specified in project it shows a version and location of pom for an according artifact resolved in maven project.
Is it OK to do it this way or I should not show any version in case it's not managed by project?

@mickaelistria
Copy link
Contributor

Any progress here?

@mickaelistria
Copy link
Contributor

Thanks!

@pbodnar
Copy link

pbodnar commented Jul 5, 2023

Thank you!

It would be great to also jump directly to the line where the given artifact version is defined and not to just open the linked pom.xml. A new feature request targeted on LemMinX, on m2e, or maybe both?

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.

4 participants