Conversation
|
Thank you! I will review as soon as I can find enough free time. The worst case scenario ir right after Easter when I have some free days though I hope to do so sooner. |
|
No problem if you don't have time in a while, gives me time to correct some failing tests regarding https://github.github.com/gfm/#tables-extension- and the examples there. |
|
@Knagis i have implemented GFM:s task lists (https://github.github.com/gfm/#task-list-items-extension-), should i merge those changed to this PR or should i make a new PR for that? |
Knagis
left a comment
Knagis
left a comment
There was a problem hiding this comment.
I finished reviewing the code. There are few files that should be reformatted so that it does not show every line as a change. Other than that the one big problem is relying on StringPart representing a whole line - which is not a guarantee.
| CommonMarkConverter.ProcessStage3(ast, str, WriteSettings); | ||
| html = str.ToString(); | ||
| } | ||
| Assert.AreEqual("<table><thead><tr><th>First Header</th><th>Second Header</th></tr></thead><tbody><tr><td>Content Cell</td><td>Content Cell</td></tr><tr><td>Content Cell</td><td>Content Cell</td></tr></tbody></table>", html); |
There was a problem hiding this comment.
Perhaps it would be better if every would be followed by a newline? Perhaps even each cell?
There was a problem hiding this comment.
This is according to the GHFM-specs, i think its better to leave it as is to have it the same as the spec.
| /// The parser will recognize syntax <c>[foo]</c>, which will be encoded in a separate AST node that the host application may evaluate as desired. | ||
| /// <summary> | ||
| /// The parser will recognize syntax <c>[foo]</c>, which will be encoded in a separate AST node that the host application may evaluate as desired. |
| /// | ||
| /// Refer to https://help.github.com/articles/organizing-information-with-tables/ for more examples | ||
| /// </summary> | ||
| GithubStyleTables = 3, |
| /// <summary> | ||
| /// The parser will recognize | ||
| /// | ||
| /// First Header | Second Header |
There was a problem hiding this comment.
add <code> around the example (I suppose that is the correct tag as otherwise the intellisense could format the example).
| @@ -0,0 +1,48 @@ | |||
| using System; | |||
| private static void MakeCell(string text, Block row, ref int offset) | ||
| { | ||
| int length = text.Length; | ||
| string trimmedText = text.TrimStart(); |
There was a problem hiding this comment.
a new string copy trimmedText should not be created - instead the whitespaces should be counted within the text and then the StringContent created from the text instance.
Though as before, it could be left as a //TODO:
| continue; | ||
|
|
||
| var lineLength = line.Length; | ||
| string actualLine = line.Source.Substring(line.StartIndex, line.Length); |
There was a problem hiding this comment.
Do not create the string copy actualLine, instead use the string part as is.
| { | ||
| if ((settings.AdditionalFeatures & CommonMarkAdditionalFeatures.GithubStyleTables) == 0) return false; | ||
|
|
||
| var parts = b.StringContent.RetrieveParts().Array; |
There was a problem hiding this comment.
the RetrieveParts() do not guarantee that they will be split on the line. A single part could contain multiple lines and also a single line could consist of multiple parts.
There was a problem hiding this comment.
Can you give me an example of when a line would be multiple parts?
| @@ -0,0 +1,234 @@ | |||
| using System; | |||
|
|
||
| System.Diagnostics.Debug.WriteLine(message, "Warning"); | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
please reformat using 4 space indent
|
What is the status of this? Does CommonMark.NET support tables yet? |
|
Due to my current family situation i havent had the time to finish this. I
hope to be able to do it during the fall.
På 18 oktober 2017 7:44:24 em Jake Burgy <notifications@github.com> skrev:
… What is the status of this? Does CommonMark.NET support tables yet?
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#111 (comment)
|
4 participants
Hi,
I took the code from #90 and tried to fix your comments.
Would close #42.
/ekblom