Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

HannesWell · 2022-10-08T09:54:51Z

This makes createProblemMarker() part of the AbstractProjectConfigurator API and therefore available to downstream implementors and makes it simpler for them to create problem markers.

@mickaelistria, @laeubi what do you think?

laeubi · 2022-10-08T10:02:08Z

I think its a good idea.

github-actions · 2022-10-08T10:08:38Z

Unit Test Results

596 tests 590 ✔️ 7m 39s ⏱️
  94 suites     6 💤
  94 files     0 ❌

Results for commit 8514ffd.

♻️ This comment has been updated with latest results.

kwin · 2022-10-09T13:34:42Z

...ipse.m2e.core/src/org/eclipse/m2e/core/project/configurator/AbstractProjectConfigurator.java

+   * </p>
+   * 
+   * @param execution the execution
+   * @param attribute the attribute to mark


It would be good to elaborate which values are valid here

I assume that you can use any attribute of the plug-ins XML configuration, but I can check that.

Maven pom.xml doesn’t use XML attributes. I have seen that element names have been given here.

There are at least combine.* attribute that Maven uses: https://maven.apache.org/pom.html#advanced_configuration_inheritance

Those are not meant here IIUC.

Actually (XML) elements are meant her, not attributes. Sorry for the confusion.
I just blindly copied that from the internal method.

HannesWell · 2022-10-09T23:00:46Z

@mickaelistria what do you think?

mickaelistria · 2022-10-10T08:10:06Z

Are there some connectors requesting this change? I don't mind growing the API if it does solve real problems; but growing the API without clear value should be avoided as it only prevents from further innovation.

HannesWell · 2022-10-10T21:50:51Z

Are there some connectors requesting this change? I don't mind growing the API if it does solve real problems; but growing the API without clear value should be avoided as it only prevents from further innovation.

Yes of course API additions should be well considered.
At the moment only the Tycho-/Bundle-Connector use them, which are now part of m2e-core. But since they used to be external think there could be more connectors that could benefit from this method.
In general I think the connectors that are now part of m2e-core should use as much as possible of the official m2e API and should only rely on internals if there is a good reason that nobody else could use it. IMHO this helps us to provide a good API for down-stream/external connectors and can function as show-case how to use it.

But personally I don't have a need for that API at the moment.

kwin · 2022-10-11T10:57:22Z

I have a need and would love to use the new methods in the configurators of https://github.com/apache/sling-ide-tooling/tree/master/eclipse/eclipse-m2e-ui/src/org/apache/sling/ide/eclipse/m2e/internal.

kwin · 2022-10-11T10:58:47Z

...ipse.m2e.core/src/org/eclipse/m2e/core/project/configurator/AbstractProjectConfigurator.java

+      facade = request.mavenProjectFacade();
+    }
+    MavenProblemInfo problem = new MavenProblemInfo(message, severity, location);
+    markerManager.addErrorMarker(facade.getPom(), IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problem);


the marker id should be made configurable (given as additional argument) as not every consumer uses that ID

Also IMavenConstants.MARKER_CONFIGURATION_ID seems to be the more reasonable default for error with the configurator due to some mojo execution configuration.

Also IMavenConstants.MARKER_CONFIGURATION_ID seems to be the more reasonable default for error with the configurator due to some mojo execution configuration.

Good point.

the marker id should be made configurable (given as additional argument) as not every consumer uses that ID

Do you want to use custom problem markers?

One problem I see here, is that in most cases consumers are probably fine with the default problem marker. But IMavenConstants.MARKER_CONFIGURATION_ID is not part of the M2E's API so we should not encourage users to use it. Instead a overload of the method without marker-type that passes that type/id is probably more suitable. It should then make no claims about the type. If necessary we can just change the default in the future.

If users want custom marker why can't they create them on their own?

If users want custom marker why can't they create them on their own?

Most of the methods/classes used in that method are internal. So to create a custom marker one a consumer would have to rely on M2E internals or would have to copy a lot of other code to rely only on Eclipse/Maven API.

I think it is about the type/id String being custom. And from M2E the convenient way to add markers to a pom is wanted.

But why? Custom marker only make sense if you add custom attributes and handle those custom, otherwise one can simply reuse the m2e one.

we end up with a much larger API than just adding a overload with one additional parameter.

There are already many parameters and adding even more for unclear use-case do not make it better I think. All one needs would probably be in SourceLocationHelper already...

OK, that's a point.
Maybe @kwin can elaborate a bit more about his use case and what would fit?

Nevertheless a createProblemMarker() method that creates IMavenConstants.MARKER_CONFIGURATION_ID problems still seems reasonable for me.

So if suitable we can still create an API around SourceLocationHelper and SourceLocation later.

But why? Custom marker only make sense if you add custom attributes and handle those custom, otherwise one can simply reuse the m2e one.

Custom markers appear in a dedicated section. There is not necessarily the need to set attributes on those. Also custom markers can be easily queried or cleared. So IMHO just exposing the marker ID as additional argument doesn't do any harm and allows this API to cover more use cases.

OK sounds like there is a use-case for that.
Instead of having more and more arguments to that method, what do you think about encapsulating the problem data into a record with static factories?

The components would be int problemSeverity, String problemMessage, String markerId and two static factory methods

create(int problemSeverity, String problemMessage)

create(int problemSeverity, String problemMessage, String markerId)

The first one uses IMavenConstants.MARKER_CONFIGURATION_ID as markerId.
The java-doc of the record should then state that only canonical constructor is considered internal and should not be called by clients (maybe with @noinstanciate) and only the static factories should be used.

Sounds good to me.

This makes createProblemMarker() part of the AbstractProjectConfigurator API and therefore available to downstream implementors.

HannesWell · 2022-10-13T18:27:12Z

@kwin do you have any more comments on this?
As said, above I think this is a reasonable start and is ready to go.
If more fine tuned marker creation is needed we can add more in subsequent PRs.

mickaelistria · 2022-10-19T06:38:16Z

...ipse.m2e.core/src/org/eclipse/m2e/core/project/configurator/AbstractProjectConfigurator.java

+   *          {@link IMarker#SEVERITY_WARNING SEVERITY_WARNING} or {@link IMarker#SEVERITY_ERROR SEVERITY_ERROR}
+   * @param problemMessage the message of the created problem marker
+   */
+  protected void createProblemMarker(MojoExecution execution, String element, ProjectConfigurationRequest request,


Could the IMarker be returned here?
What if one wants to create the marker on the value of the element? Can't we give it a text position instead, and an helper to locate an element in a mojoExecution?

Could the IMarker be returned here? What if one wants to create the marker on the value of the element?

In general IMavenMarkerManager could be changed to return the marker too.
Creating the marker at the value is not yet supported, but in general could be considered.

Can't we give it a text position instead, and an helper to locate an element in a mojoExecution?

Yes Christoph suggested that as well before.
And after some rethinking about it, it is probably the better way for a generalization because IIRC for markers of custom Ids m2e just creates them using the Eclipse Resources API. So there is no benefit in providing an extra API for that. But I have to check that again.
However I think for a simple case a convenient API method like this would be useful nevertheless.

However I think for a simple case a convenient API method like this would be useful nevertheless.

There is uncertainity with this API that needs to be covered before it can be claimed "API-quality" (ie we won't want to change it for a long time), and this comes by nature of using the "element" name:

How to place a marker on the value or a subset of the value? Or on some attribute (some mojo configuration may like it?

What happens is the element is repeated in the configuration?

What happens if there is no such element in the configuration?

...

All those questions wouldn't be raised if we clearly separate locating the marker vs creating it. If it is clear that locating the marker is the important part, and not creating it (because Platform API is enough), then instead of API to create a marker, we should have an API to reoslve locations and use already existing satisfying APIs from Platform to create it.

laeubi · 2023-03-06T08:16:40Z

@mickaelistria @HannesWell @kwin do you plan to finish this one?

HannesWell · 2023-03-06T08:19:34Z

Yes I do, altough it isn't my no1 priority at the moment.

G-Ork · 2024-01-09T11:29:42Z

Looks like i could use this to improve the Maven Settings dialog in eclipse preferences to list all validation problems with the settings or toolchain files. Actually only the first error is squeezed into the upper text output in the dialog. All info is lost after closing the dialog. But in this case i need more a factory like IMavenMarkerManager. But as the context an config is loaded for execution if needed it may possible to hook in there.

kwin reviewed Oct 9, 2022

View reviewed changes

HannesWell force-pushed the problemMarker branch from 8b8be2e to 44e43e0 Compare October 10, 2022 21:43

kwin reviewed Oct 11, 2022

View reviewed changes

HannesWell force-pushed the problemMarker branch 3 times, most recently from a2164db to 3ea6ea2 Compare October 12, 2022 18:49

Move createProblemMarker() to AbstractProjectConfigurator and clean up

8514ffd

This makes createProblemMarker() part of the AbstractProjectConfigurator API and therefore available to downstream implementors.

HannesWell force-pushed the problemMarker branch from 3ea6ea2 to 8514ffd Compare October 13, 2022 18:24

HannesWell requested review from laeubi, mickaelistria and kwin October 13, 2022 18:25

mickaelistria reviewed Oct 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

HannesWell commented Oct 8, 2022

laeubi commented Oct 8, 2022

github-actions bot commented Oct 8, 2022 •

edited

Loading

kwin Oct 9, 2022

HannesWell Oct 9, 2022

kwin Oct 10, 2022

mickaelistria Oct 10, 2022

kwin Oct 10, 2022

HannesWell Oct 10, 2022

HannesWell commented Oct 9, 2022

mickaelistria commented Oct 10, 2022

HannesWell commented Oct 10, 2022

kwin commented Oct 11, 2022

kwin Oct 11, 2022 •

edited

Loading

kwin Oct 11, 2022

HannesWell Oct 12, 2022

laeubi Oct 12, 2022

HannesWell Oct 12, 2022

laeubi Oct 12, 2022

HannesWell Oct 12, 2022

kwin Oct 14, 2022

HannesWell Oct 18, 2022

kwin Oct 18, 2022

HannesWell commented Oct 13, 2022

mickaelistria Oct 19, 2022

HannesWell Oct 19, 2022

mickaelistria Oct 19, 2022

laeubi commented Mar 6, 2023

HannesWell commented Mar 6, 2023

G-Ork commented Jan 9, 2024

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

Are you sure you want to change the base?

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

Conversation

HannesWell commented Oct 8, 2022

laeubi commented Oct 8, 2022

github-actions bot commented Oct 8, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell commented Oct 9, 2022

mickaelistria commented Oct 10, 2022

HannesWell commented Oct 10, 2022

kwin commented Oct 11, 2022

kwin Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi commented Mar 6, 2023

HannesWell commented Mar 6, 2023

G-Ork commented Jan 9, 2024

github-actions bot commented Oct 8, 2022 •

edited

Loading

kwin Oct 11, 2022 •

edited

Loading