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

Improved emphasis parser #301

Merged
merged 8 commits into from
Feb 9, 2019
Merged

Conversation

MihaZupan
Copy link
Collaborator

@MihaZupan MihaZupan commented Jan 29, 2019

This allows for emphasis parsing with min/max above 2 characters.

I marked IsDouble on EmphasisInline as Obsolete as described below. This should not be a breaking change for existing parsers/renderers.
https://github.com/lunet-io/markdig/blob/d232e847dd1a6822d72cb09740511ee84729100a/src/Markdig/Syntax/Inlines/EmphasisInline.cs#L24-L36

I also changed the way adding a CreateEmphasisInline works (since there can no longer be a bool isStrong identifier), so now there's no need to save the previous one, attempt the new one and revert to the old one on callsites, instead adding the delegate to the list, returning null if not providing an emphasis. CreateEmphasisInlineDelegate is marked as obsolete in favour of TryCreateEmphasisInlineDelegate. Note that this shouldn't be a breaking change, since for now the parser will attempt to use the old delegate first.
https://github.com/lunet-io/markdig/blob/42b201d8c2f599b9f3a2f4ba6f89a412be45d745/src/Markdig/Extensions/CustomContainers/CustomContainerExtension.cs#L29-L36

internal static class LegacyHelper
{
[MethodImpl(MethodImplOptionPortable.AggressiveInlining)]
public static bool HasFlag(this Enum enumeration, Enum flags)
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't we use instead the new generic constraint for that? (where TEnum : System.Enum, likely requiring the project to use C#7.3)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not know about this addition.
Added the generic constraint now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just added the helper method for net35 since I find the old way of checking for flags unintuitive.

@xoofx
Copy link
Owner

xoofx commented Feb 8, 2019

LGTM

public static bool HasFlag<TEnum>(this TEnum enumeration, TEnum flags) where TEnum : Enum
{
Debug.Assert(enumeration.GetType() == flags.GetType());
ulong enumVal = Convert.ToUInt64(enumeration);
Copy link
Owner

Choose a reason for hiding this comment

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

actually, looking at the code behind, Convert.ToUInt64 typically incurs an allocation (cast IConvertible) which is just super bad. I would prefer that we remove this method entirely. It's not that annoying in the codebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Afaik there's no way to see how many people are using the NuGet by framework version right?

Copy link
Owner

Choose a reason for hiding this comment

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

no unfortunately

@xoofx xoofx merged commit 80b918e into xoofx:master Feb 9, 2019
@MihaZupan MihaZupan deleted the improved-emphasis-parser branch March 11, 2019 17:33
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.

2 participants