Fix repetitive diagnosis output in init response

[buraksenn]

In the issue #1812 it was explained and demonstrated clearly that init response was outputting duplicate diagnosis error. I have added Merge method to diagnostics to not add exact equal diagnostics to prevent duplicates.

Resolves #1812

Target Release

1.9.0

Checklist

• I have read the contribution guide.
• I have not used an AI coding assistant to create this PR.
• I have written all code in this PR myself OR I have marked all code I have not written myself (including modified code, e.g. copied from other places and then modified) with a comment indicating where it came from.

Go checklist

• I have run golangci-lint on my change and receive no errors relevant to my code.
• I have run existing tests to ensure my code doesn't break anything.
• I have added tests for all relevant use cases of my code, and those tests are passing.
• I have only exported functions, variables and structs that should be used from other packages.
• I have added meaningful comments to all exported functions, variables, and structs.

[github-actions]

Reminder for the PR assignee: If this is a user-visible change, please update the changelog as part of the PR.

[ollevche]

Hey @buraksenn, thank you for working on this issue!

Ignoring backDiags when we print earlyConfDiags may require users to run init multiple times just to find out there are more errors that need fixing.

I would suggest trying to consolidate those diags even if they are errors. It needs to be done delicately so we don't silence any useful messages, that are not duplicated.

That will also help with failed tests, since we don't change the existing behaviour.

Happy to answer any questions and discuss potential concerns!

[buraksenn]

Hey @buraksenn, thank you for working on this issue!

Ignoring backDiags when we print earlyConfDiags may require users to run init multiple times just to find out there are more errors that need fixing.

I would suggest trying to consolidate those diags even if they are errors. It needs to be done delicately so we don't silence any useful messages, that are not duplicated.

That will also help with failed tests, since we don't change the existing behaviour.

Happy to answer any questions and discuss potential concerns!

That made sense to me as well but I saw that we are not consolidation error level if source is nil so thought that this would be the fix. Then the resolution would be to expand consolidateWarnings or add another method such as consolidateErrors to prevent duplicates right @ollevche ?

[cam72cam]

Hmm, I've done some related work on one of my branches: 032c9ff#diff-cb9acfeab1fa4de42a8453d53172343075c9db7dfbd41d59569050760f455132

I can take some time today to get that PR ready for review. It could make sense to rebase this work on top of my changes once they are reviewed and merged.

[Ericwww]

I would also prefer the opinion that consolidate duplicated error. For some details, we could have a discussion on how to judge diagnostics is equal (or duplicated) 😄

I noticed Christian have made some progress.

[cam72cam]

Long term, we should revise how and where we are loading configuration (only once). That depends on a lot of refactoring in the command package that we have not had time to start or review.

The question is do we want to do a "generic" merge like in #1894 or do we want to have a more explicit "merge between these two sets" logic, which might make more sense here.

[ollevche]

Sorry, I missed @cam72cam was working on a similar change! We should definitely base #1812 work on top of what Christian is doing (#1894).

I agree it makes more sense to have some explicit merge logic, that will result in a unique diag list from the previous two (backDiags and earlyConfDiags).

[cam72cam]

I agree it makes more sense to have some explicit merge logic, that will result in a unique diag list from the previous two (backDiags and earlyConfDiags).

In that case @ollevche, it probably wouldn't make sense for this PR to be based on mine, though it could be used as a reference.

[buraksenn]

I want to contribute in long-term but since this is my first commit and I'm not familiar with the project structure and architecture decisions I don't want to proceed without asking in this case. So I'm not sure which approach I should take right now. I would be glad to do the work related to consolidating etc but it would be great to clarify the approach. @cam72cam should I wait for your PR to be merged to continue?

[cam72cam]

@buraksenn You should be able to continue on this branch with the approach of merging two different lists of diagnostics, removing any exact duplicates. This would probably look something like:
diags = diags.Append(earlyConfDiags.Merge(backDiags))

[buraksenn]

@buraksenn You should be able to continue on this branch with the approach of merging two different lists of diagnostics, removing any exact duplicates. This would probably look something like: diags = diags.Append(earlyConfDiags.Merge(backDiags))

Thanks @cam72cam , I will continue with this. Appreciate it

[buraksenn]

@ollevche can you review again if possible?

[ollevche]

@buraksenn I'll take a look at the PR today. Thank you for working on this one!

[ollevche]

Thank you for the contribution!

It looks pretty good, special kudos for adding a test. However, I think we need to change the comparison logic to something similar to ConsolidateWarnings' implementation.

Left a comment with more details.

[cam72cam]

Thanks @buraksenn !