-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added OriginalMarkdown Property #2737
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
Conversation
Added logic for OriginalMarkdown in different block types
|
Looks good to me. The only question/concern I have is the additional memory used by the new property. Is there a way to use a reference to a substring instead, using span maybe? |
@nmetulev I've not yet used Your idea would be to store the resulting string as |
This PR seems inactive. @joejoe87577 Do you need help to complete this issue? |
I'm just waiting for another review as this pr needs two reviews. |
Sorry for the late response @joejoe87577. I just realized that Span is not available in .NET Standard 2.0 so we probably can't use it. However, I'm still wondering if there is a way to avoid having to keep a copy of the string in each element and instead generating it as needed. For example, what if we keep a reference as the range in the original string only and generate the string in ToString()? |
We should also add some Unit Tests to our Markdown Unit Test project to ensure that the right results are returned as expected. |
This PR seems inactive. @joejoe87577 Do you need help to complete this issue? |
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.
The code works well, the only small issue is that the OriginalMarkdown property isn't filled on List children, but only on the list itself.
@Sergio0694 what are your thoughts on Span and perf here and how we could optimize this? |
@michael-hawker I'll have a look at the specific package and this PR and get back to you! Also @nmetulev about In fact, I was just looking around the markdown parser in this package the other day, and was thinking that it could definitely use some For instance, most inline parsers have a line like this: Which could basically always be replaced by a |
After taking a look at this PR I think we could just remove the overhead caused by allocations when setting the new The idea would be, instead of using This is possible since the type has a special handling when the wrapped object is /// <summary>
/// Gets or sets the <see cref="ReadOnlyMemory{T}"/> for <see cref="OriginalMarkdown"/>.
/// </summary>
internal ReadOnlyMemory<char> OriginalMarkdownMemory { get; set; }
private string _OriginalMarkdown;
/// <summary>
/// Gets the original Markdown the element was parsed from.
/// </summary>
public string OriginalMarkdown => _OriginalMarkdown ??= OriginalMarkdownMemory.ToString(); Note: the This is more of an API-design preference issue, the main point is to avoid allocations for users that don't actually need this new |
@Sergio0694 thanks for all your input here! I know this kind of expands the scope of this original PR for this feature, but I believe if I understand correctly your comments, but if we more broadly apply these techniques to provide the |
Moving out of 6.1 release as think refactoring this would be a lot of work and may be superseded by issue #3200 if we just end up adopting a more robust parser. Hopefully a library like Markdig supports this type of feature already? @MihaZupan |
Markdig exposes a If this Toolkit exposes a wrapper over the AST, it is free to expose a lazily-allocated (as @Sergio0694 suggested) substring if the user requests it. I would hold off on exposing more API until a potential migration has been prototyped. |
@joejoe87577 thanks for taking an initial look at his. I think we should just roll this effort into #3200, I've added your original issue as a requirement there. Are you ok with us closing this PR? |
@michael-hawker yeah you can close this PR. Thanks. |
Added logic for OriginalMarkdown in different block types
Issue: #2729
PR Type
What kind of change does this PR introduce?
Feature
What is the current behavior?
As described in #2729
What is the new behavior?
Added new property
OriginalMarkdown
toMarkdownBlock
and added logic to extract the original markdown from the parsed string in all block types.PR Checklist
Please check if your PR fulfills the following requirements:
Other information