Skip to content

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

Merged
Sheng Chen (jdneo) merged 16 commits into
microsoft:mainfrom
mamilic:main
Oct 31, 2024
Merged

Sheng Chen (jdneo) merged 16 commits into
microsoft:mainfrom
mamilic:main

Conversation

@mamilic

Copy link
Copy Markdown
Contributor

This MR referes to the issue, #857

@mamilic

Marko Milic (mamilic) commented Oct 16, 2024 •

Copy link
Copy Markdown
Contributor Author

Hi Sheng Chen (@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?

Comment thread src/views/containerNode.ts Resolved
@jdneo Sheng Chen (jdneo) added the enhancement New feature or request label Oct 17, 2024
Comment thread CONTRIBUTING.md Resolved
Comment thread src/views/packageRootNode.ts Resolved
@mamilic

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Comment thread CONTRIBUTING.md Resolved
Comment thread src/views/dependencyDataProvider.ts Resolved
Comment thread src/views/containerNode.ts Resolved
Comment thread src/views/PrimaryTypeNode.ts Resolved
Comment thread src/views/dependencyDataProvider.ts Resolved
Comment thread src/views/dependencyDataProvider.ts Resolved
Comment thread src/views/containerNode.ts Resolved
@jdneo

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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
Copy Markdown
Member

I'm not hurry, just take your time 😉

@mamilic

Copy link
Copy Markdown
Contributor Author

Sheng Chen (@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
Copy Markdown
Member

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
Copy Markdown
Member

I tested on my mac but I cannot reproduce it.

@jdneo Sheng Chen (jdneo) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@jdneo

Copy link
Copy Markdown
Member

Marko Milic (@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.

@mamilic

Copy link
Copy Markdown
Contributor Author

Sheng Chen (@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