Abstracted descriptor matching.

[michibat01]

This will permit to have different id's for two different recordIssue preserving the correct Descriptor (Icon, DetailsTableModel, DisplayName).
To do that it is enough to override the "describes" method in the class extending ReportScanningTool,
e.g.:
@OverRide
public boolean describes(String id){
return id.startsWith("my-scanning-tool");
}

[uhafner]

That would imply that you need a prefix for the new IDs. And we would need to override that method at multiple places. Wouldn't it be much simpler if we would additionally store the original tool ID side by side and then use it as fallback if the label descriptor with the new ID is empty?

[abramobagnara]

If we will store the original tool ID what would be the point to search descriptor using also the custom ID? Isn't it perfectly pointless?

[uhafner]

Hmm, I'm not sure if I understand your point. Maybe it would be helpful if you can create a small integration tests that shows the problem. You can take one of the tests here as a template: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/plugin/src/test/java/io/jenkins/plugins/analysis/warnings/steps/StepsITest.java#L462
This is required for pull requests anyway.

From your description I expected that you want something like

recordIssues id: "one" tool: javaDoc(pattern:'**/*issues.txt', reportEncoding:'UTF-8')
recordIssues id: "two" tool: javaDoc(pattern:'**/*issues.txt', reportEncoding:'UTF-8')

That should show up with 2 URLs one and two and the Java icon of the id javaDoc and the name "JavaDoc"?
This could be achieved if I store in the individual results the id one and the original id javadoc.

But maybe I misunderstand your plan? Should the pull request above already show the results without any additional work for RecordScanningTool classes?

[michibat01]

That would imply that you need a prefix for the new IDs. And we would need to override that method at multiple places. Wouldn't it be much simpler if we would additionally store the original tool ID side by side and then use it as fallback if the label descriptor with the new ID is empty?

    public StaticAnalysisLabelProvider create(final String toolId, final String id, @CheckForNull final String name) {
        DescriptorExtensionList<Tool, ToolDescriptor> extensions = jenkins.getDescriptorsFor(Tool.class);
        for (ToolDescriptor descriptor : extensions) {
            if (descriptor.getId().equals(id)) {
                return createNamedLabelProvider(descriptor.getLabelProvider(), name);
            }
        }
        for (ToolDescriptor descriptor : extensions) {
            if (descriptor.getId().equals(toolId)) {
                return createNamedLabelProvider(descriptor.getLabelProvider(), name);
            }
        }

        List<StaticAnalysisToolFactory> factories = jenkins.getExtensionsFor(StaticAnalysisToolFactory.class);
        for (StaticAnalysisToolFactory factory : factories) {
            Optional<StaticAnalysisLabelProvider> labelProvider = factory.getLabelProvider(id);
            if (labelProvider.isPresent()) {
                return createNamedLabelProvider(labelProvider.get(), name);
            }
        }

        return new StaticAnalysisLabelProvider(id, name);
    }

Is this what you mean with the above message?
This means to have to propagate the extra argument through all the callers chains.
I've also found some place where this cannot be done: e.g.

    public PropertyStatistics getDetails(final String propertyName) {
        Function<String, String> propertyFormatter;
        if ("fileName".equals(propertyName)) {
            propertyFormatter = new BaseNameMapper();
        }
        else if ("origin".equals(propertyName)) {
            propertyFormatter = origin -> new LabelProviderFactory().create(origin,
                    getIssues().getNameOfOrigin(origin)).getName();
        }
        else {
            propertyFormatter = Function.identity();
        }
        return new PropertyStatistics(report, newIssues, propertyName, propertyFormatter);
    }

My original proposal is far less invasive and permits to a Descriptor to be associated to a set of custom Id's.
Current scanning tool are not involved, but if they choose to be associated to more custom Id (different from default one) they can override the added 'describes' method (that is a possibility and not a duty).
Once said that I'm willing to follow your guidelines about the approach you feel better to solve the original issue (that is exactly what you describe in your last message)

[michibat01]

As Abramo wrote above, another possibility is to use always the toolId to create StaticAnalysisLabelProvider,
but I fear that this might create problems with StaticAnalysisToolFactory

[uhafner]

Are you planning to add a small test that shows the new functionality?

[michibat01]

Are you planning to add a small test that shows the new functionality?

Yes I will.
I am currently busy with something else, but as soon as I have some free time I work there

[michibat01]

I added the test.
Note that i have changed the default describes method to do matching with prefix.
Another possibility is that default matching is done by equality and that only interested ReportScanningTool override the describes method to allow for more matches (as before).

[codecov]

Codecov Report

Merging #897 (215b4de) into master (bfef1b5) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 215b4de differs from pull request most recent head ced8e8c. Consider uploading reports for the commit ced8e8c to get more accurate results

@@             Coverage Diff              @@
##             master     #897      +/-   ##
============================================
+ Coverage     79.89%   79.98%   +0.08%     
+ Complexity     1467     1423      -44     
============================================
  Files           246      237       -9     
  Lines          5417     5240     -177     
  Branches        424      411      -13     
============================================
- Hits           4328     4191     -137     
+ Misses          939      895      -44     
- Partials        150      154       +4     

Impacted Files	Coverage Δ
...gins/analysis/core/model/LabelProviderFactory.java	100.00% <100.00%> (ø)
...a/io/jenkins/plugins/analysis/core/model/Tool.java	71.05% <100.00%> (-3.37%)	⬇️
...ns/plugins/analysis/warnings/RegisteredParser.java	73.33% <0.00%> (-26.67%)	⬇️
.../io/jenkins/plugins/analysis/warnings/JavaDoc.java	80.00% <0.00%> (-20.00%)	⬇️
...s/plugins/analysis/core/model/InfoErrorDetail.java	80.00% <0.00%> (-20.00%)	⬇️
.../io/jenkins/plugins/analysis/warnings/PhpStan.java	66.66% <0.00%> (-16.67%)	⬇️
.../analysis/warnings/groovy/ParserConfiguration.java	54.28% <0.00%> (-8.51%)	⬇️
...ins/analysis/core/portlets/IssuesTablePortlet.java	87.62% <0.00%> (-8.45%)	⬇️
...ins/plugins/analysis/core/model/SourcePrinter.java	67.12% <0.00%> (-8.37%)	⬇️
...jenkins/plugins/analysis/core/util/FileFinder.java	80.76% <0.00%> (-7.70%)	⬇️
... and 44 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfef1b5...ced8e8c. Read the comment docs.

[michibat01]

@uhafner
Are we ready to merge now?

[uhafner]

I did not find the time yet to check if that does not break anything else. Did you check what happens if we have two parser IDs that share the same prefix? (Or where one parser ID is prefix of another one?)

Otherwise the PR looks fine!

[michibat01]

I did not find the time yet to check if that does not break anything else. Did you check what happens if we have two parser IDs that share the same prefix? (Or where one parser ID is prefix of another one?)

Otherwise the PR looks fine!

It takes the first one it finds.
I think it would be better to avoid putting two parser ids with these ids relations.

[michibat01]

@uhafner
Did you have time to look at the changes? how do you want to proceed?

[uhafner]

@uhafner
Did you have time to look at the changes? how do you want to proceed?

As mentioned above, the code looks fine. I just creates a new problem:

It takes the first one it finds.
I think it would be better to avoid putting two parser ids with these ids relations.

Currently some parsers already share the same prefix, this cannot be changed without breaking existing jobs:
https://github.com/jenkinsci/analysis-model/blob/master/SUPPORTED-FORMATS.md

[uhafner]

So I am somewhat unsure what to do. I think when I am finished with my current feature implementation (see #889) I will have some more time to dig into that problem.