-
Notifications
You must be signed in to change notification settings - Fork 2
Optimize project structure for cases without Java files #42
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
base: main
Are you sure you want to change the base?
Conversation
Test Results 42 files ±0 42 suites ±0 1d 3h 25m 19s ⏱️ - 20m 18s For more details on these failures, see this check. Results for commit dd7b8b2. ± Comparison against base commit 9542833. ♻️ This comment has been updated with latest results. |
|
@guw I updated the PR, and my test environment configuration is as follows: 001/.bazelversion 001/.eclipse/.bazelproject |
| * @return the temporary file name if created, or null if no temporary file was needed | ||
| * @throws CoreException | ||
| */ | ||
| private String ensureJavaFileExistsInPackage(BazelTarget bazelTarget, List<String> srcs) throws CoreException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does no java file exists? Is it generated by some other target and copied into the source tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project structure was maintained in this way; it wasn't generated. Specifically, there are no other Java files in the same directory as BUILD in the source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's figure out a way without temp. files then. I think the logic should be fixed to handle this case, i.e. .java sources within nested directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method ensures that at least one Java file in the same directory as the BUILD file can be detected in the event of multiple nested levels. If not, temporary Java files are created and then deleted after use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but it does not belong here.
The logic to detect source directories is here:
Line 114 in 9542833
| public void analyzeSourceDirectories(MultiStatus result, |
It looks like it's missing to identify that particular setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the logic here. However, it's quite lengthy. I wanted to directly reuse the logic of detectPackagePath, so I used the temporary file creation approach. If I didn't create a temporary file, should I add more code to handle this case in the analyzeSourceDirectories method? Or do you think refactoring this method would be a reasonable thing to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all the weirdness trying to map from Bazel srcs to Java source directories required by JDT.
The problem is that bazel query does not return directories but only list of files. Thus, we try to identify the directory. Depending on the provisioning strategy this could be more or less challenging.
The method is quite lengthy and increasingly challenging to maintain at this point. I have not had time to investigate a different implementation, which could also be better unit tested. I am not going to stop you from refactoring it perhaps into a separate heuristics object. I can support you with providing requirements. Although I will probably forget a few edge cases, which I need to look up from Git commit history.
| deps = [ | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the next step is to add some assertions to
Line 42 in 9542833
| public class Workspace001Test_Provisioning { |
Fixed: #39
@guw