-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix inline code in left navigation and h1 rendering #290
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpicks other than that it LGTM!
{ | ||
var title = !string.IsNullOrEmpty(_navigationTitle) ? _navigationTitle : Title; | ||
return string.IsNullOrEmpty(title) ? null : Helpers.Markdown.StripMarkdown(title); | ||
} | ||
private set => _navigationTitle = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strip once when setting instead of each time we get the property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static class Markdown | ||
{ | ||
public static string StripMarkdown(string markdown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm partial to making these extension methods.
public static class Markdown | |
{ | |
public static string StripMarkdown(string markdown) | |
public static string class MarkdownStringExtensions | |
{ | |
public static string StripMarkdown(this string markdown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get | ||
{ | ||
var title = !string.IsNullOrEmpty(_navigationTitle) ? _navigationTitle : Title; | ||
return string.IsNullOrEmpty(title) ? null : Helpers.Markdown.StripMarkdown(title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return string.IsNullOrEmpty(title) ? null : Helpers.Markdown.StripMarkdown(title); | |
return string.IsNullOrEmpty(title) ? null : title.StripMarkdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -46,7 +46,11 @@ public DocumentationGroup? Parent | |||
public string? Title { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe introduce a TitleRaw
property thats the markdown so that we can assume Title
and NavigationTitle
are sanitized.
Then when rendering we can use TitleRaw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unsure about this. This caused changes in various places, where it's not entirely clear to me if it expects a raw markdown string or sanitized title.
Was the assumption that it's only a sanitized title during the first implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here: https://github.com/elastic/docs-builder/pull/290/files#r1927019403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
public static string StripMarkdown(string markdown) | ||
{ | ||
using var writer = new StringWriter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For funsies tried to see if pooling the StringBuilder()
this instantatiates each time had an impact but its very minimal:
private static readonly ObjectPool<StringBuilder> StringBuilderPool = new DefaultObjectPool<StringBuilder>(new StringBuilderPooledObjectPolicy());
public static string StripMarkdownPooled(string markdown)
{
var sb = StringBuilderPool.Get();
try
{
using var writer = new StringWriter(sb, CultureInfo.InvariantCulture);
Markdig.Markdown.ToPlainText(markdown, writer);
return writer.ToString().TrimEnd('\n');
}
finally
{
StringBuilderPool.Return(sb);
}
}
Method | Mean | Error | Gen0 | Gen1 | Allocated |
---|---|---|---|---|---|
StripMarkdown | 1.963 us | NA | 0.4829 | 0.0029 | 3.95 KB |
StripMarkdownPooled | 1.706 us | NA | 0.4435 | 0.0027 | 3.63 KB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, thank you!
How did you actually measure this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark dot Will add a general benchmarking project at some point in the next week so we can utilize this more.
and: https://www.elastic.co/guide/en/ecs-logging/dotnet/current/benchmark-dotnet-data-shipper.html on CI 😄
get => _titleRaw; | ||
private set | ||
{ | ||
Title = value?.StripMarkdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant it reverse e.g:
Title is the sanitized version.
public string TitleRaw { get; private set; }
public string? Title
{
get => _title;
private set
{
_title = value?.StripMarkdown();
TitleRaw = value;
}
}
That way the change is not that pervasive, the only place we need access to the markdown is in the rendering slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I think I got it right now.
35719a0
to
e00d555
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Closes #235
Closes #311