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

Add message codes for BuildCheck suggestion diags #10924

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

JanKrivanek
Copy link
Member

Fixes #10501

Context

BuildCheck suggestions didn't have the codes and formatting of warnings and errors.

Changes Made

Unified the diagnostics creation code

Copy link
Contributor

@maridematte maridematte left a comment

Choose a reason for hiding this comment

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

Will this changes need to be reflected on the binlog viewer too? I know the process of adding to it but not when we're removing something.

@JanKrivanek
Copy link
Member Author

Will this changes need to be reflected on the binlog viewer too? I know the process of adding to it but not when we're removing something.

Good question.

tl;dr;: it's fine here :-)

In this specific case we are not removing any data definition - the binlog OM stays same.
We are removing couple methods writing a specific types of data to the binlog - 2 of them were already dead (writing BuildCheck error/warning) and the third was used - but there was no reason for differentiating BuildCheck message in a binlog, while BuildCheck error nor warnings were differentiated.

Copy link
Member

@YuliiaKovalova YuliiaKovalova 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

Copy link
Contributor

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

LGTM

@JanKrivanek JanKrivanek merged commit 1c41c5d into main Nov 18, 2024
10 checks passed
@JanKrivanek JanKrivanek deleted the proto/buildcheck-message-code branch November 18, 2024 10:50
Copy link

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

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

minor comments

src/Build/BuildCheck/API/BuildCheckResult.cs Show resolved Hide resolved
src/Framework/BuildMessageEventArgs.cs Show resolved Hide resolved
{
this.code = code;
this.file = file;
this.lineNumber = lineNumber;

Choose a reason for hiding this comment

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

are there invalid ranges for either line or column numbers?

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.

BuildCheck suggestions do not start with code
5 participants