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

HtmlState: restore FormattedMode if still in pre after char ref #1414

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

stroborobo
Copy link
Contributor

@stroborobo stroborobo commented Nov 19, 2021

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.

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``() =
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

@stroborobo stroborobo changed the title HtmlState: restore FormattedMode if still in pre after char ref [WIP] HtmlState: restore FormattedMode if still in pre after char ref Nov 20, 2021
@stroborobo
Copy link
Contributor Author

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

@stroborobo stroborobo changed the title [WIP] HtmlState: restore FormattedMode if still in pre after char ref HtmlState: restore FormattedMode if still in pre after char ref Nov 23, 2021
@@ -275,13 +273,15 @@ module internal HtmlParser =
{ Attributes : (CharList * CharList) list ref
CurrentTag : CharList ref
Content : CharList ref
HasFormattedParent: bool ref
Copy link
Collaborator

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
Copy link
Collaborator

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 :)

Copy link
Contributor Author

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?

@cartermp
Copy link
Collaborator

Weird that this failure is only on Windows: Drops whitespace outside pre

@stroborobo
Copy link
Contributor Author

Oh right, the formatter inserts Environment.NewLine, totally forgot about it. I'll change the test to expect those as well.

Copy link
Collaborator

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thank you!

@cartermp cartermp merged commit afb67a7 into fsprojects:main Nov 23, 2021
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