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

Display SonarDelphi errors instead of failing scan #67

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

fourls
Copy link
Collaborator

@fourls fourls commented Oct 10, 2024

This PR refactors the handling of errors that occur within SonarDelphi during analysis. Previously, any error-level logging would cause the analysis to fail with a fatal error, which is inaccurate - SonarDelphi is quite resilient to errors and can conduct a successful partial analysis in most cases an error appears. This new approach communicates the logs directly to the client along with any results, leaving the user free to determine how important the error is / continue using DelphiLint while they identify the problem.

A new "View Last Analysis Log" option is available in the "Analyze" dropdown:

image

Clicking this after an analysis shows a log window with options to save a log file or report an issue on integrated-application-development/sonar-delphi:

image

If the analysis produces errors, then the following shows up (clicking opens the log):

image

If the analysis produces warnings, then the following shows up (clicking opens the log):

image

Example of a log with errors:

image

@fourls fourls requested a review from Cirras October 10, 2024 03:02
@fourls fourls linked an issue Oct 10, 2024 that may be closed by this pull request
3 tasks
Copy link
Collaborator

@Cirras Cirras left a comment

Choose a reason for hiding this comment

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

Great change.

Surfacing errors and warnings in this way is a nice win for consistency with standard SonarDelphi scans, and much less disruptive when things do go wrong.
I also really like the Analysis Log UI and hope the Report a bug on GitHub button will drive some useful bug reports to the upstream SonarDelphi project.

Testing went well.

  • Should we have more visual feedback on the Errors and Warnings buttons?
    • The little magnifying glass is cute, but easy to miss.
    • Tooltip on hover saying "View analysis log" or similar
    • Background color change on hover, like with other buttons.
  • The GitHub icon in the Analysis Log is a nice touch, but there's some white pixels around it that look a little funny. BMP antialiasing issue?
    image

Code changes are mostly good to go with a few nitpicks, see below.

@fourls fourls force-pushed the show-sonardelphi-warns branch from 01eac33 to 27da72f Compare October 10, 2024 23:34
@fourls
Copy link
Collaborator Author

fourls commented Oct 10, 2024

(rebase)

@fourls fourls force-pushed the show-sonardelphi-warns branch from 27da72f to 616acbc Compare October 11, 2024 00:26
@fourls
Copy link
Collaborator Author

fourls commented Oct 11, 2024

Updated now based on feedback.

Log description:

image

GitHub icon (dark theme):

image

GitHub icon (light theme):

image

Should we have more visual feedback on the Errors and Warnings buttons?

  • The little magnifying glass is cute, but easy to miss.
  • Tooltip on hover saying "View analysis log" or similar
  • Background color change on hover, like with other buttons.

VCL is surprisingly restrictive with what it lets you do with buttons. I've added a tooltip, but the background color is totally dependent on the theme. In light mode, for example, it actually does darken the buttons:

light_mode_errors

There's not too much I can do without implementing a custom control. I don't think what we have now will be too much of a problem - it's so colourful and angry that I expect most people will click on it anyway.

@fourls fourls requested a review from Cirras October 11, 2024 00:26
@fourls fourls force-pushed the show-sonardelphi-warns branch from 616acbc to 689d992 Compare October 11, 2024 00:28
@fourls fourls force-pushed the show-sonardelphi-warns branch from 689d992 to fac04a8 Compare October 11, 2024 02:41
Copy link
Collaborator

@Cirras Cirras left a comment

Choose a reason for hiding this comment

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

Looks good to me. 🎉

@fourls fourls merged commit 8109e63 into master Oct 11, 2024
4 checks passed
@fourls fourls deleted the show-sonardelphi-warns branch October 11, 2024 04:06
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.

Provide a means to resolve analysis bugs in included libraries
2 participants