-
-
Notifications
You must be signed in to change notification settings - Fork 456
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
CommonMark 0.29, DisableHeadings extension, AutoLinkOptions #327
Conversation
@@ -0,0 +1,35 @@ | |||
// Generated: 2019-03-17 11:31:23 |
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 think that we should start to use the .generated.cs
postfix for these files, as it is becoming confusing to have multiple of them around and not being sure if they are generated or not.
ValidPreviousCharacters = "*_~("; | ||
} | ||
|
||
public string ValidPreviousCharacters { get; 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.
I'm a bit worried to introduce a mutable option class here, as a pipeline is supposed to be immutable after a build...
I don't recall if we have many cases already like this in the code base, but we should ideally strive for making these classes immutable
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.
Looking at other extensions, PipeTable
, Media
and JiraLink
have mutable option classes while SmartyPantOptions
is unmodifiable.
Would you rather these all have readonly fields and extra constructor params?
I can't really think of a clean way to enforce immutability otherwise (unless extensions made an internal copy and changes wouldn't propagate after the initial setup)
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.
yeah, C# is really poorly helping us here unfortunately... ok let it as it is for this one
We will have to later decide what to do with all these mutable classes and if we want to break them
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.
A property like befd1ca#diff-b59084d198ccb5dc29ab3f390ae3fcc7R15 falls under the same category them
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.
yes, as stated in the original issue #5 about immutable, these are open problems...
return BlockState.Continue; | ||
} | ||
} | ||
// Copyright (c) Alexandre Mutel. All rights reserved. |
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.
Weird, why this file is being marked as completely modified?
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.
MB, apparently I let VS set \r\n line endings for this file.
Actual changes: gist diff
LGTM, thanks @MihaZupan for working so quickly on the 0.29 specs changes! |
for (int i = line.Start; i <= line.End; i++) | ||
{ | ||
char c = line.Text[i]; | ||
if (c == '`') |
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.
Shouldn't this using openingCharacter
instead here?
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.
This checks that there are no backtick characters in the info string if the opening character is also a backtick, as from 0.29 backticks in info strings are allowed for tilde fenced blocks.
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.
oops sorry, yes, let me check again the file (messy diff in github)
Closes #83
Closes #145
Adds Spec file system for Normalize and PlainText renderers
Adds
DisableHeadings()
extension method on the pipeline builder to disable ATX and Setex headings.Adds
AutoLinkOptions
withOpenInNewWindow
andUseHttpsForWWWLinks
.