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

[READY] Handle multiple identical diagnostics in YcmShowDetailedDiagnostic popup #4219

Merged

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Feb 3, 2024

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

If a line for which detailed diagnostics are requested contains more than a single diagnostic with the same message, YCM will try options.pop( 'col' ) more than once.

We do not need to really iterate through the diagnostics once we have found the first one that is a match. If there's only one diag with the matching message, looping beyond that diagnostic is just a waste of time. If there's multiple diagnostics with the same message, it does not matter which one we display in the popup.

Hence, the added break at the end of the loop.


This change is Reviewable

Copy link

codecov bot commented Feb 3, 2024

Codecov Report

Merging #4219 (cea2458) into master (c55e732) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4219      +/-   ##
==========================================
+ Coverage   89.71%   89.74%   +0.02%     
==========================================
  Files          34       34              
  Lines        4424     4425       +1     
==========================================
+ Hits         3969     3971       +2     
+ Misses        455      454       -1     

@bstaletic bstaletic force-pushed the detailed-diag-popup-revenge-of-the-sith branch from cfed814 to 5e40ea8 Compare February 3, 2024 16:09
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained

@bstaletic bstaletic force-pushed the detailed-diag-popup-revenge-of-the-sith branch from 5e40ea8 to cea2458 Compare February 3, 2024 16:42
If a line for which detailed diagnostics are requested contains
more than a single diagnostic with the same message, YCM will try
`options.pop( 'col' )` more than once.

We do not need to really iterate through the diagnostics once we have
found the first one that is a match. If there's only one diag with the
matching message, looping beyond that diagnostic is just a waste of
time. If there's multiple diagnostics with the same message, it does not
matter which one we display in the popup.

Hence, the added break at the end of the loop.
@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Feb 3, 2024
Copy link
Contributor

mergify bot commented Feb 3, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 0497bb0 into ycm-core:master Feb 3, 2024
12 of 13 checks passed
@bstaletic bstaletic deleted the detailed-diag-popup-revenge-of-the-sith branch February 3, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants