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

Java: Fix gradle project root detection #1749

Merged

Conversation

puremourning
Copy link
Member

@puremourning puremourning commented Jul 31, 2024

Typical "modern" layout from gradle init:

  • settings.gradle
  • app/build.gradle
  • ...

If you open app/src/java/.../Foo.java, we incorrectly pick app/ as the project root because we find first build.gradle then keep looking only for that file. Instead we should keep looking for any gradle file.

Now, when searching for the project root, we continue to search the varioius file names, but we tag each file name with the "kind" and keep searching for other files of the same kind rather than just the same name.

Meanwhile, we prefer finding the gradle/maven project. The reason for this is that jdt.ls spams .project files all over the codebase which makes any project you've previously opened look like an eclipse project. This is fine if the project root was previously correctly determined and/or the projects properly imported, but means that we never self-correct. So prefer finding the non-native project types so that we pick the correct root in most cases.


This change is Reviewable

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained


ycmd/completers/java/java_completer.py line 45 at r1 (raw file):

PATH_TO_JAVA = None

PROJECT_FILE_TAILS = {

making this a dictionary makes the sequence undefined. Either revert to list or use OrderedDict.

@puremourning puremourning force-pushed the java-fix-gradle-root-detection branch from 71e4d78 to 219baa5 Compare July 31, 2024 12:32
Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained


ycmd/completers/java/java_completer.py line 45 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

making this a dictionary makes the sequence undefined. Either revert to list or use OrderedDict.

done

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Added a test using bare grade init project

Reviewable status: 0 of 2 LGTMs obtained

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewed 26 of 26 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewed 26 of 26 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)


ycmd/completers/java/java_completer.py line 45 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

done

Iteration order of regular dictionaries is defined to be the same as insertion order, since... python 3.7. We only support python 3.8 and newer and even python 3.8 is nearing EOL.

https://mail.python.org/pipermail/python-dev/2017-December/151283.html

A list of tuples is an odd data type, when dict works. If you think we should be more explicit about ordering, then there's still OrderedDict.

@bstaletic bstaletic force-pushed the java-fix-gradle-root-detection branch from 219baa5 to 779a711 Compare August 23, 2024 18:00
Typical "modern" layout from `gradle init`:

- settings.gradle
- app/build.gradle
- ...

If you open app/src/java/.../Foo.java, we incorrectly pick app/ as the
project root.

Now, when searching for the project root, we continue to search the
varioius file names, but we tag each file name with the "kind" and
keep searching for other files of the same *kind* rather than just the
same name.

Meanwhile, we prefer finding the gradle/maven project. The reason for
this is that jdt.ls spams .project files all over the codebase which
makes any project you've previously opened look like an eclipse project.
This is fine if the project root was previously correctly determined
and/or the projects properly imported, but means that we never
self-correct. So prefer finding the non-native project types so that we
pick the correct root in _most_ cases.
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.94%. Comparing base (5de7052) to head (779a711).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1749      +/-   ##
==========================================
+ Coverage   95.92%   95.94%   +0.02%     
==========================================
  Files          84       84              
  Lines        8462     8464       +2     
  Branches      163      163              
==========================================
+ Hits         8117     8121       +4     
+ Misses        295      293       -2     
  Partials       50       50              

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Aug 23, 2024
Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

Copy link
Contributor

mergify bot commented Aug 23, 2024

Thanks for sending a PR!

1 similar comment
Copy link
Contributor

mergify bot commented Aug 23, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 22a5c84 into ycm-core:master Aug 23, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants