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.

Sign up for GitHub

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

Jump to bottom

Fix #1915: Allow result message to be truncated #1932

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Fix #1915: Allow result message to be truncated #1932

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8b3fe6
Fix #1915: Allow result message to be truncated
Jun 24, 2020
1887b04
Address PR feedback; update release history.
Jun 25, 2020
64f3feb
Restore accidentally deleted Autogenerated files.
Jun 25, 2020
2908166
Restore erroneously deleted const.
Jun 25, 2020
98e313b
Add comments for "horizontal ellipsis".
Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Sarif/ExtensionMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ public static string GetMessageText(this Result result, ReportingDescriptor rule
return GetMessageText(result, rule, concise: false);
}

public static string GetMessageText(this Result result, ReportingDescriptor rule, bool concise = false)
public static string GetMessageText(this Result result, ReportingDescriptor rule, bool concise = false, int maxLength = 200)
Copy link
Member

@michaelcfanning michaelcfanning Jun 25, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

200 [](start = 128, length = 3)

is 120 or 128 a better default? #Resolved

Copy link
Author

Choose a reason for hiding this comment

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

When I filed the issue I said 120; when I coded it, I worried it was too short. But OK, anybody who calls the API and decides it's not enough can always increase it.

In reply to: 445546182 [](ancestors = 445546182)

Copy link
Member

Choose a reason for hiding this comment

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

The key question is 'too short for what'? i.e., under what circumstances would someone be likely to request a concise string and expect something around 200 characters. If you have a scenario in mind, let me know. I've mostly been thinking about flat list views in IDEs as a driver for this API, or work item titles. In these contexts, 200 chars seems more than what's required. Open to thinking about other UX scenarios if you have one.

In reply to: 445558418 [](ancestors = 445558418,445546182)

{
if (result == null)
{
Expand Down Expand Up @@ -333,6 +333,10 @@ public static string GetMessageText(this Result result, ReportingDescriptor rule
if (concise)
{
text = GetFirstSentence(text);
if (text.Length > maxLength)
{
text = text.Substring(0, maxLength) + "...";
Copy link
Member

@michaelcfanning michaelcfanning Jun 25, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

... [](start = 59, length = 3)

Why aren't you using the ellipsis char? #Resolved

}
}

return text;
Expand Down
24 changes: 24 additions & 0 deletions src/Test.UnitTests.Sarif/SarifExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,5 +113,29 @@ public void SarifExtensions_Result_GetMessageText_Concise()
string actual = result.GetMessageText(rule, concise: true);
Assert.Equal(expected, actual);
}

[Fact]
public void SarifExtensions_Result_GetMessageText_Concise_Truncated()
{
var result = new Result
{
Message = new Message
{
Id = "ruleStr1"
}
};

var rule = new ReportingDescriptor
{
MessageStrings = new Dictionary<string, MultiformatMessageString>
{
["ruleStr1"] = new MultiformatMessageString { Text = "First sentence is very long. Second sentence." }
}
};

const string Expected = "First sentence is ve...";
string actual = result.GetMessageText(rule, concise: true, maxLength: 20);
Copy link
Member

@michaelcfanning michaelcfanning Jun 25, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

compute this from your expected value (length - length of your trailing dots or ellipses. #Resolved

Assert.Equal(Expected, actual);
}
}
}