feat(submodule): support project that has multiple submodules. #73
Conversation
|
Sorry for the possible confusion around the failing build! If you could rebase or merge master into your branch (prefer rebase, but up to you), then the build will probably work as expected 🙂 |
There was a problem hiding this comment.
Tested it with some simple submodule projects, and without submodules (to verify that nothing breaks). Works fine, and fixes the issue so submodules finally work! 🙂 Nice work! 😄
Will give @fwcd some time to have a look before merging in case he have some comments
| .filter { Files.isDirectory(it) } | ||
| .collect(Collectors.toSet()) | ||
| private fun sourcesRootsOf(projectRoot: Path): Set<Path> = | ||
| Files.walk(projectRoot, 2) // root project and submodule |
There was a problem hiding this comment.
Hm, ideally I'd like to not hardcode the 2 levels, since there are lots of edge cases where this wouldn't work (project in subfolder, nested subprojects, etc.). The current logic is already somewhat hacky and in the medium-term having a clean solution would almost certainly pay off, in the sense that we shouldn't have to account for all of these scenarios manually
There was a problem hiding this comment.
What we should probably do is query Maven/Gradle to find source files and fall back to walking the source tree only if everything else fails
There was a problem hiding this comment.
I agree with you on the ideally part here. A Maven/Gradle query seems like a much better long term solution 🙂
Development velocity is not super fast in this project, so might take some time. We have several people who want to use the project to debug multi-module codebases, which are atm not possible. Merging this PR might not be perfect, but it moves us slowly a step forward.
My suggestion: We make an issue of your suggestion for a more permanent fix, with a link to the current solution (just a permalink to the code changed in this PR). In this issue we write what you have written above about a Maven/Gradle query and the fallback. Part of a fallback might also be an optional input with maximum source root depth or something? Anyway, we can put information like that in the issue-text. Does that sound okay to you @fwcd ? 🙂
There was a problem hiding this comment.
I agree with @themkat 's suggestion.
I knew the PR was not perfect at that time, but it can resolve the problem temporarily. I tried to query Maven/Gradle and classic Eclipse project source directories by parsing their's description file or using LSP information, but it is a bit complex.
I think the long-term solution have to query project's source construction, but it need some time to do.
There was a problem hiding this comment.
Yeah I suppose supporting this use case for now is better than what we have. In any case, this snippet is in dire need of refactoring since it makes too many assumptions about the user's project layout, but let's move that to a future PR.
There was a problem hiding this comment.
Created an issue for this #77.
|
Thank you @alps2006 ! Let's merge this now 🙂 |
The error below will occur when a project has multiply submodules.
I add some codes to support the project that has multiply submodules.