-
Notifications
You must be signed in to change notification settings - Fork 289
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
HtmlState: restore FormattedMode if still in pre after char ref #1414
HtmlState: restore FormattedMode if still in pre after char ref #1414
Conversation
At the same time the assumption that the code element is formatted is not correct. They are often used at the same time though and with that comes another issue: keeping FormattedMode in elements nested into pre.
@@ -805,15 +805,16 @@ let ``Can parse pre blocks``() = | |||
result |> should equal [ "\r\n This code should be indented and\r\n have line feeds in it" ] | |||
|
|||
[<Test>] | |||
let ``Can parse code blocks``() = | |||
let html = "<code>\r\n let f a b = a * b\r\n f 5 6 |> should equal 30</code>" | |||
let ``Can parse pre blocks with char refs``() = |
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 removes the code blocks test - could you add that test back as well?
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.
Sure, it would test for invalid behavior though since code is an inline element. Should I restore the behavior for code anyways and open another issue for elements nested in pre?
Hey @cartermp, since you didn't say anything I implemented my earlier suggestion with the bool ref instead. My assumption was not correct in the original implementation anyways, so it didn't really work well, test was flawed also :/ But this seems to work, at least for the fixed up test and my use case of glueing together different parts of fsharp.github.io/fsharp-core-docs |
@@ -275,13 +273,15 @@ module internal HtmlParser = | |||
{ Attributes : (CharList * CharList) list ref | |||
CurrentTag : CharList ref | |||
Content : CharList ref | |||
HasFormattedParent: bool ref |
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.
An alternative is that this can be a mutable HasFormattedParent
to avoid the ref
, but I don't think that's terribly important.
if x.IsFormattedTag then | ||
x.HasFormattedParent := not isEnd | ||
else | ||
x.HasFormattedParent := !x.HasFormattedParent || x.IsFormattedTag |
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.
as an example if this is mutable
-able, this could just be
x.HasFormattedParent <- x.HasFormattedParent || x.IsFormattedTag
The codebase is fairly old :)
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.
Hm, I just meant to be consistent, should I update it?
Weird that this failure is only on Windows: |
Oh right, the formatter inserts Environment.NewLine, totally forgot about it. I'll change the test to expect those as well. |
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.
Thank you!
At the same time the assumption that the code element is formatted is not correct. They are often used at the same time though and with that comes another issue: keeping FormattedMode in elements nested into pre.
I'm not sure how to approach this, I've never worked with refs like here before. Would it be ok to drop the FormattedMode and add an IsFormatted: bool ref instead? And update that when entering/exiting pre elements.