-
Notifications
You must be signed in to change notification settings - Fork 142
Account for closing html tags during diff #455
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
Conversation
test/test_site/testUtil/diffHtml.js
Outdated
| * <div/> | ||
| */ | ||
| const endsWithOpeningTag = (fragment) => { | ||
| let numUnopenedTag = 0; |
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.
Sounds odd as it sounds like we're saying one can close a tag before opening it.
Is it reasonable to assume that this is +/-1? How about hasUnmatchedClosingBracket?
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 was thinking of a fragment like
<panel header="<a>Test</a>">Here there are 2 > s before a <.
Maybe i will use numUnmatchedClosingBracket?
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.
Ah right, thanks.
test/test_site/testUtil/diffHtml.js
Outdated
| */ | ||
| const diffHtml = (expected, actual) => { | ||
| // console.log(expected); | ||
| // console.log(actual); |
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.
Remove?
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.
My bad 😅
97f5a54 to
757daeb
Compare
| } | ||
| if (fragment[i] === '>') { | ||
| return false; | ||
| numUnmatchedClosingBracket += 1; |
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.
Maybe more intuitive to +1 before -1, and check for 0 explicitly.
if > {
+1
} else if < {
if 0 {
return true
}
-1
}
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've updated the commit
61ea3e5 to
2e06b06
Compare
acjh
left a comment
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.
Thanks for the fix!
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [ ] Documentation update
• [X] Bug fix
• [ ] New feature
• [ ] Enhancement to an existing feature
• [ ] Other, please explain:
What is the rationale for this request?
Address the bug found in #454 where the diffing logic incorrectly identifies a diff outside a file path.
What changes did you make? (Give an overview)
The fragment that causes this is (note i've formatted it for readability)
To decide if the diff
\is found in a path, it checks if the diff>or<first.<is found, its within a html tag - diff is in path>is found, assume not within html tag - diff is not in path (and this is incorrect for the case above)For the last case, instead of assuming not within html tag, we look to check if the
>is "closed" by a<. If so, continue searching for<.Provide some example code that this change will affect:
Is there anything you'd like reviewers to focus on?
Testing instructions:
For code snippet in above found above in
test/test_site/expected/index.html, change the path separator and check that there is no diff error reported by runningnpm run test.