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

Fix maven repository discovery #518

Merged
merged 3 commits into from
Oct 21, 2023
Merged

Conversation

calamont
Copy link
Contributor

Related to #430

Fixes the Maven repository discovery by checking if the candidate paths have a /repository directory. An exception is thrown if no valid repository paths are found. The order of discovery is maintained as before:

  • $MAVEN_REPOSITORY
  • $MAVEN_HOME
  • $M2_HOME
  • $HOME/.m2

Fixes the maven repository discovery by checking if the candidate paths
have a `/repository` directory. An exception is thrown if no valid
repository paths are found.
@fwcd fwcd added the maven Related to the language server's support for Maven projects label Oct 11, 2023
Copy link
Collaborator

@themkat themkat 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 🙂

calamont and others added 2 commits October 18, 2023 18:50
@calamont
Copy link
Contributor Author

Thanks for the reviews @themkat and @fwcd 🙂 How do you want the git history to look for this project? I can squash the new commits for the PR locally and force push a single commit to merge if you want.

@themkat
Copy link
Collaborator

themkat commented Oct 20, 2023

Thanks for the reviews @themkat and @fwcd 🙂 How do you want the git history to look for this project? I can squash the new commits for the PR locally and force push a single commit to merge if you want.

No strong opinions from me 🙂 I squash PRs anyway when merging.

@calamont
Copy link
Contributor Author

Ok cool. I'll let you guys squash when merging then 👍

@themkat
Copy link
Collaborator

themkat commented Oct 21, 2023

All PR comments seems to be resolved, so merging. Thank you @calamont ! 😄

@themkat themkat merged commit d10d8b1 into fwcd:main Oct 21, 2023
Comment on lines +22 to +24
?: throw KotlinLSException(
"No repositories found at \$MAVEN_REPOSITORY, \$MAVEN_HOME, \$M2_HOME or \$HOME/.m2"
)
Copy link
Owner

Choose a reason for hiding this comment

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

I think the exception might have broken all clients that didn't have a Maven repository (e.g. #564). We should really be a bit more careful with silently changing the type of something, this had to be optional for the class path resolver logic to work properly...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maven Related to the language server's support for Maven projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants