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

Consider profiles in the maven.config as well #553

Merged

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Feb 16, 2024

One can define profiles in the .mvn/maven.config file but currently these are ignored by lemminx leading to unexpected or even fault project models.

This now parses the profile option form the maven config and pass them to the building request.

Like we do in m2e already, @mickaelistria do you can review?

@mickaelistria
Copy link
Contributor

That looks good to me. However, I recommend if possible that you try to add some tests for such features as lemminx-maven is a pretty "mutable" project that's relatively prone to regressions. The good news is that writing tests is very easy.

@laeubi
Copy link
Member Author

laeubi commented Feb 16, 2024

Can you suggest a simple testcase that checks for a property present in the project? It then should be straight forward to add such test.

@mickaelistria
Copy link
Contributor

that checks for a property present in the project

This has no strong meaning in LSP land, as the grain is Text/UI.
I think you can add a test to Hover, which would hover in some ${property} which is supposed to show the property value, and create a pom with a profile that sets this value to something like set-from-profile, so the output of hover should then show the profile-enabled value for this property by default.

@laeubi
Copy link
Member Author

laeubi commented Feb 16, 2024

Ok is there any such hover test I can then reuse? Another would be error markers, I can use a version with a property and it should not show any errors...

@mickaelistria
Copy link
Contributor

Ok is there any such hover test I can then reuse?

You can get inspiration from MavenPropertyHoverTest. There is no test that deals with profiles currently nor maven.config, so you'll probably need to create a some new test resources to consume.

@laeubi laeubi force-pushed the consider_profiles_in_maven_config branch 2 times, most recently from 5221f99 to 7a8281e Compare February 16, 2024 16:36
@laeubi
Copy link
Member Author

laeubi commented Feb 16, 2024

@mickaelistria I tried to add some test but they failed, as there is not much context beside that it seems I can't get the hover do you have any hint what might be wrong or causing that? Any additional logs I could look at?

@mickaelistria
Copy link
Contributor

You can configure Java logging as usual to enable more logging, but can't you just debug those tests? That would be much more efficient.

@laeubi
Copy link
Member Author

laeubi commented Feb 18, 2024

I now used just the debugger and found that lemminx is not using an ExpressionEvaluator for the hover but collecting properties on its own, so I need to enhance this to support user properties as well... I also noticed that this modifies the original project properties what not seems correct.

@laeubi laeubi force-pushed the consider_profiles_in_maven_config branch from 7a8281e to 2db37ef Compare February 18, 2024 09:56
@laeubi
Copy link
Member Author

laeubi commented Feb 18, 2024

@mickaelistria I got it now working with the test, if this is merged, is there a chance to have a new release of lemminx?
@HannesWell is currently preparing m2e release and as I understand we need to update the lemminx.jar there to get changes picked up...

One can define profiles in the .mvn/maven.config file but currently
these are ignored by lemminx leading to unexpected or even fault project
models.

This now parses the profile option form the maven config and pass them
to the building request.

Also adding two test cases that check properties are taken into account.
@laeubi laeubi force-pushed the consider_profiles_in_maven_config branch from 2db37ef to d99bda8 Compare February 18, 2024 13:29
@mickaelistria mickaelistria merged commit e1e70d0 into eclipse:master Feb 18, 2024
3 checks passed
@HannesWell
Copy link
Member

@mickaelistria do you plan a new LemMinX release any time soon? I would like to release m2e 2.6.0 actually not later than tomorrow evening and it would be nice if this could be included too.

@mickaelistria
Copy link
Contributor

do you plan a new LemMinX release any time soon?

Done at https://github.com/eclipse/lemminx-maven/releases/tag/0.11.1

@HannesWell
Copy link
Member

do you plan a new LemMinX release any time soon?

Done at https://github.com/eclipse/lemminx-maven/releases/tag/0.11.1

Thanks for the fast release. I'm about to update m2e to it:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants