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

ux - display maven and gradle dependencies with pattern 'groupId:artifactId:version ' #859

Merged
merged 16 commits into from
Oct 31, 2024

Conversation

mamilic
Copy link
Contributor

@mamilic mamilic commented Oct 16, 2024

This MR referes to the issue, #857

@mamilic
Copy link
Contributor Author

mamilic commented Oct 16, 2024

Hi @jdneo , this is my view of the implementation for #857, though I am not fully satisfied with changes, as I had to introduce getLabel() method to ExplorerNode. It seems that this only works for Maven, as when gradle project is loaded, the jdtls is not recognizing dependencies as Gradle, instead it is. :/
image

Could you please take a look and let me know what I might have done better? Also, is there a requirement to test his functionality?

src/views/containerNode.ts Outdated Show resolved Hide resolved
@jdneo jdneo added the enhancement New feature or request label Oct 17, 2024
CONTRIBUTING.md Show resolved Hide resolved
src/views/packageRootNode.ts Outdated Show resolved Hide resolved
@mamilic
Copy link
Contributor Author

mamilic commented Oct 18, 2024

@microsoft-github-policy-service agree

CONTRIBUTING.md Show resolved Hide resolved
@mamilic mamilic force-pushed the main branch 2 times, most recently from 62f1f6e to 1ed0f67 Compare October 28, 2024 12:05
src/views/containerNode.ts Outdated Show resolved Hide resolved
@jdneo
Copy link
Member

jdneo commented Oct 29, 2024

Overall looks good to me. Are you willing to add a test case in the maven-suite/projectView.test.ts? I can also handle that you have no time to do that.

@mamilic
Copy link
Contributor Author

mamilic commented Oct 29, 2024

Overall looks good to me. Are you willing to add a test case in the maven-suite/projectView.test.ts? I can also handle that you have no time to do that.

I'm definitely willing to add a test case. It would be a great learning opportunity for me, though it might take some time. That said, it depends on how quickly we’re looking to merge this. Let me know what works best!

@jdneo
Copy link
Member

jdneo commented Oct 29, 2024

I'm not hurry, just take your time 😉

@mamilic
Copy link
Contributor Author

mamilic commented Oct 30, 2024

@jdneo, 2 tests are failing, though I am not sure if that is intended, as the latest build from main branch reflect the same result, https://github.com/microsoft/vscode-java-dependency/actions/runs/10659297304/job/29541618151

@jdneo
Copy link
Member

jdneo commented Oct 30, 2024

It's unlikely the failures are caused by this PR. Please give me some time to do some investigation (I was occupied by some other stuff recently).

If I cannot get some finding I'll merge this PR first.

@jdneo
Copy link
Member

jdneo commented Oct 31, 2024

I tested on my mac but I cannot reproduce it.

Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM

@jdneo
Copy link
Member

jdneo commented Oct 31, 2024

@mamilic Thanks for your contribution!

I appended a commit to pass the test. I guess it's related with some timing issue. Since gradle project needs to download gradle distribution during import in CI jobs. It will take longer time.

@jdneo jdneo merged commit b839f1e into microsoft:main Oct 31, 2024
4 of 6 checks passed
@jdneo jdneo added this to the 0.24.1 milestone Oct 31, 2024
@mamilic
Copy link
Contributor Author

mamilic commented Oct 31, 2024

@jdneo, thank you for your patience and guidence on this issue, and do apologies for taking so long. Next step is to enable this for gradle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants