-
Notifications
You must be signed in to change notification settings - Fork 679
fix: Ensure more doc/html links stay in doc/html #10022
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
base: main
Are you sure you want to change the base?
Conversation
As far as I can tell, the code that was supposed to rewrite references was inoperative. It just glued the same URL back together. This version is effective. The links to RFCs in the header of documents were being missed. I did not change the RFC link that appears in boilerplate. I also update the text of the link if it is the same as the destination, to avoid surprises. This also applies the fix unconditionally, so that the treatment is consistent regardless of what preference is chosen.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10022 +/- ##
==========================================
- Coverage 88.55% 88.54% -0.01%
==========================================
Files 317 317
Lines 42343 42385 +42
==========================================
+ Hits 37496 37530 +34
- Misses 4847 4855 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@larseggert, this is another one of your changes. Maybe you can help tell if this makes sense. |
larseggert
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.
If it works now, it makes sense :-)
We really need unit tests for all this datatracker JS code.
| document.activeElement.blur(); | ||
|
|
||
| // keep RFC links within the datatracker | ||
| const rfclink = /^https?:\/\/[^\/\.]*.(?:rfc-editor|ietf).org\/.*\/((?:rfc|std|bcp)\d+)$/; |
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.
Perhaps should limit the subdomains that are treated as RFC links here. A link to https://notes.ietf.org/.../bcp7890 isn't an RFC link. It's also not a great example as it's not likely to appear at all, but the point is real RFC links are at a more constrained set of hosts.
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 wasn't confident that I could enumerate the locations. Is it just www.ietf.org and datatracker.ietf.org and www.rfc-editor.org? The deployment architecture of this stuff (and its history) are a bit opaque to me.
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'll have a look, but imo it'd be better to start off conservatively
As far as I can tell, the code that was supposed to rewrite references was inoperative. It just glued the same URL back together. This version is effective.
The links to RFCs in the header of documents were being missed. I did not change the RFC link that appears in boilerplate.
I also update the text of the link if it is the same as the destination, to avoid surprises.
This also applies the fix unconditionally, so that the treatment is consistent regardless of what preference is chosen.