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

Move report generation out of AnalysisDetails #577

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

mc1arke
Copy link
Owner

@mc1arke mc1arke commented Apr 10, 2022

The access to metrics from a Pull Request analysis is exposed through an
AnalysisDetails instance, which also provides the ability to extract a
formatted report. As a number of the metrics used in the summary report
need to be retrieved through various additional DAOs, and as the
resolution of URLs for links and images requiring access to core
Sonarqube configuration, AnalysisDetails holds references to a high
number of classes from Sonarqube's core. Some of those core Sonarqube
classes are also referenced directly in some decorators which don't make
use of the summary report but need equivalent metrics to those shown in
the summary which means some searching logic is duplicated across the
plugin.

This change pulls the report generation into a ReportGenerator class,
with the report being an interim set of collected metrics that each
decorator can extract required information, or generate a formatted
report from.

The access to metrics from a Pull Request analysis is exposed through an
`AnalysisDetails` instance, which also provides the ability to extract a
formatted report. As a number of the metrics used in the summary report
need to be retrieved through various additional DAOs, and as the
resolution of URLs for links and images requiring access to core
Sonarqube configuration, `AnalysisDetails` holds references to a high
number of classes from Sonarqube's core. Some of those core Sonarqube
classes are also referenced directly in some decorators which don't make
use of the summary report but need equivalent metrics to those shown in
the summary which means some searching logic is duplicated across the
plugin.

This change pulls the report generation into a `ReportGenerator` class,
with the report being an interim set of collected metrics that each
decorator can extract required information, or generate a formatted
report from.
@sonarcloud
Copy link

sonarcloud bot commented Apr 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.3% 97.3% Coverage
0.0% 0.0% Duplication

@mc1arke mc1arke merged commit ce92893 into master Apr 10, 2022
@mc1arke mc1arke deleted the analysis-details-refactor branch April 10, 2022 17:44
dashboardUrl,
coverage,
analysisSummary.getDashboardUrl(),
analysisSummary.getNewCoverage(),
Copy link

@Karql Karql Mar 21, 2023

Choose a reason for hiding this comment

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

Hi, @mc1arke this change is regresion for #225

There is already open issue for it: #285

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @Karql! Rather than placing similar comments at different points in the project, could you raise an issue stating the problem and replication steps, and provide any links to changes you believe have caused the issue at that point? This way other people have visibility of the issue being reported, and can follow any discussion on it.

Copy link

@Karql Karql Mar 21, 2023

Choose a reason for hiding this comment

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

Sorry I wanted to link these places together but forgot to past link to issue here.

Full details can be found here: #285

I edited my comments to be more specific.

I didn't want to annoy you 😉

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.

2 participants