-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Unclosed html tag lint #77119
Unclosed html tag lint #77119
Conversation
5479228
to
0f0632f
Compare
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 might have gone overboard on the comments 😆
src/test/rustdoc-ui/unclosed-tags.rs
Outdated
/// <script> | ||
//~^ ERROR Unclosed HTML tag `unknown` | ||
//~^^ ERROR Unclosed HTML tag `script` | ||
/// <img><input> |
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.
Can you add some more tests?
- test these outside of the
<script>
to make sure they still don't warn - add some nested tags that should be closed and make sure they don't warn inside of
<script>
- add nested tags that should be closed and make sure they do warn outside of
<script>
(maybe<del>
since that's what inspired me to look at the issue)
6d8e95a
to
c043897
Compare
Updated! |
We should absolutely not be doing our own HTML parsing here, HTML parsing is extremely complicated even in cases like this one where HTML is rare. If we can get this info from pulldown that's fine, but a hand rolled bespoke HTML parser is all but guaranteed to have problems. |
A different approach: perhaps we can look for very specific strings, e.g. |
Actually thinking about this more I guess it's fine if we parse a very limited subset of HTML. @jyn514 what is your take on this? I'd rather not do anything brittle here. |
I think we absolutely shouldn't stabilize the lint with a bespoke HTML parser. I think it's ok to land this as Before merging we should definitely reach out to pulldown-cmark about how hard it would be to add this info in the API. |
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.
Just a nit regarding the lint docs :)
c043897
to
c86c223
Compare
Updated! |
54d6b20
to
f9546af
Compare
Updated! |
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.
The issue is that the reexport item itself should be here, which isn't the case currently for some reason. In my opinion, this check is valid.
Ok, I opened #77230 for this. No need to block this PR on it.
f9546af
to
2ce2f0a
Compare
Updated! |
The last thing is adding a test for tags inside Other than that you just need to run
|
This comment has been minimized.
This comment has been minimized.
So should I mark this PR as fix or not? |
Let's remove it for now so we can get the initial implementation in, we can always close the issue later. r=me with that done. |
@bors r+ We can make fixes in follow up PRs if necessary. |
📌 Commit d3b7b7e has been approved by |
Hmm, I agree that we don't want this code to get too complicated. So yeah, maybe just catching some common cases might be the best we should do. Ideally we could actually fix close the unclosed tags so the rendering of the rest of the page isn't completely broken but that might be getting way too complicated. I do think #67799 should to be left open at least until this lint is stabilized and warn by default though. At the very least we need a tracking issue for doing that and reusing #67799 seems like the easiest choice. |
Agreed. I'll let @Manishearth update the issue then. |
⌛ Testing commit d3b7b7e with merge f2ef98f6f9ac42571c84c5f0d68c9d295f2216d9... |
💔 Test failed - checks-actions |
I can't see why it failed. Maybe it's a CI issue? cc @pietroalbini |
Yeah, I always get confused when it fails at "mark the job as a failure". Where are the logs of the actual failure? |
The weirdness was probably https://www.githubstatus.com/incidents/t34640ccrmfs. |
Oh okay. Well, let's try again then! @bors: retry |
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Test successful - checks-actions, checks-azure |
Part of #67799.
I think @ollie27 will be interested (@Manishearth too since they opened the issue ;) ).
r? @jyn514