-
Notifications
You must be signed in to change notification settings - Fork 31
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
Consider properties defined in .mvn/maven.config #552
Consider properties defined in .mvn/maven.config #552
Conversation
lemminx-maven/src/main/java/org/eclipse/lemminx/extensions/maven/project/MavenProjectCache.java
Outdated
Show resolved
Hide resolved
Thanks, that's a good proposal. Just 1 addition suggested. |
Currently lemminx does not consider properties that are defined in the .mvn/maven.config and therefore gives errors or otherwhise wrong results when parsing a project. This adds support for detecting and reading these properties from the .mvn/maven.config Fix eclipse-lemminx#549 Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
c2f686c
to
0bef665
Compare
If it is good in general I would polish it a bit (e.g. caching the result of config parsing maybe), possibly it would make sense to reload the file if save is performed and on typing always use a cached variant I assume that |
Another issue is that a maven.config can provide profiles and different settings, this is currently not taken into account as well but better be solved in a different PR. |
I think the gain here would be very small because loading projects involves much longer work, so I wouldn't invest in optimizing this particular bit.
I'm not sure I get the question correctly; but my undestanding is that if pom.xml is not the only input (maven.config is too), then the cached project, and it's consumers, needs to be removed from cache when the maven.config changes too. But as it's not a too frequent case to modify maven.config (vs pom.xml), I wouldn't make it a immediate requirement to immediately react on updates.
OK. I let you look at what you want to change now, but IMO it's already in a good state to be merged. Please ping whenever you think it's ready to be merged. |
@mickaelistria if you are fine with it I'm fine with it as well, I tested it locally and it works as expected for me. |
Thank you for this very valuable addition! |
Currently lemminx does not consider properties that are defined in the .mvn/maven.config and therefore gives errors or otherwhise wrong results when parsing a project.
This adds support for detecting and reading these properties from the .mvn/maven.config
Fix #549