feat: Properly link to cross-referenced RFCs

[logan-stytch]

Fixes #38

Handles cross-RFC section references by identifying these before RFC linking, adding special markers, proceeding with normal RFC linking, and then removing the cross-RFC markers once finished.

To be honest, there are a bunch of very nasty regexes in here, so it was hard to be 100% certain that this was correct. I added a number of new test cases to tests.py to hopefully validate that this is working as expected.

[jennifer-richards]

Sorry for the delay but just gave this a look. I have a couple questions/suggestions but don't have a firm request for change and I'm not quite ready to sign off on an approval.

We should run this over a bunch of documents that don't have these sorts of references and confirm that they don't change. The new tests are great, but I worry that we don't have coverage for the negative cases where the new sort of link isn't wanted. Looking for those should happen before this merges IMO.

Thank you for your effort and for the contribution!

[logan-stytch]

@jennifer-richards, thanks for the feedback!

Looking for those should happen before this merges IMO

Dumb question, but any advice on how I should go about doing this? Do you also think it would be useful to import a few of these into the repo as bigger tests so future updates can also make use of them?

[jennifer-richards]

Dumb question, but any advice on how I should go about doing this? Do you also think it would be useful to import a few of these into the repo as bigger tests so future updates can also make use of them?

Very fair questions. I didn't actually mean to ask you to do it - I was thinking it'd be something we would do. The actual test would just be gathering up "some" RFCs, running the original and patched versions, and comparing the output. These shouldn't change except in the cases you're following.

@kesara may have a better idea whether this is necessary or how to pick a representative set of inputs.

Importing some files into the repo is a possibility, but as a corpus to manually test against and extract specific tests of functionality. The ietf-tools/xml2rfc has tests that amount to "the output for this input shouldn't change," and that turns out to be a tricky test to maintain.

[kesara]

@logan-stytch, @jennifer-richards
May be set of txt RFCs before RFC8650 might be a good pick.
For example https://www.rfc-editor.org/rfc/rfc8649.txt
Output can be compared with https://www.rfc-editor.org/rfc/rfc8649.html
There are differences already, but I guess it's a good place to start.

Apart from that randomly selected new RFCs and couple of Internet-Drafts should be good.

Sorry I don't have any specific suggestions.

[jennifer-richards]

Output can be compared with https://www.rfc-editor.org/rfc/rfc8649.html
There are differences already, but I guess it's a good place to start.

As long as this particular change isn't adding new deviations, I'd call it good enough. I.e., I'd compare rfc2html's action on the .txt file before and after this PR rather than comparing to the rfc-editor.org HTML

[logan-stytch]

I downloaded RFC 7817 and RFC 8649. Here are the diffs of each. For each one, I compared main to this branch. There are a couple instances of my branch turning   into actual space characters, but otherwise this seemed to pass the 👀 check.

RFC 7817:

❯ diff test_rfcs/rfc7817.txt.OLD.html test_rfcs/rfc7817.txt.NEW.html
15,18c15,18
<    ManageSieve clients.  It replaces <a href="#section-2.4">Section 2.4</a> (Server Identity Check)
<    of <a href="./rfc2595">RFC 2595</a> and updates <a href="#section-4.1">Section 4.1</a> (Processing After the STARTTLS
<    Command) of <a href="./rfc3207#section-11.1">RFC 3207, Section&nbsp;11.1</a> (STARTTLS Security Considerations)
<    of <a href="./rfc3501">RFC 3501</a>, and <a href="#section-2.2.1">Section 2.2.1</a> (Server Identity Check) of <a href="./rfc5804">RFC 5804</a>.
---
>    ManageSieve clients.  It replaces <a href="./rfc2595#section-2.4">Section 2.4 (Server Identity Check)
>    of RFC 2595</a> and updates <a href="./rfc3207#section-4.1">Section 4.1 (Processing After the STARTTLS
>    Command) of RFC 3207</a>, <a href="./rfc3501#section-11.1">Section 11.1 (STARTTLS Security Considerations)
>    of RFC 3501</a>, and <a href="./rfc5804#section-2.2.1">Section 2.2.1 (Server Identity Check) of RFC 5804</a>.
28c28
<    Internet Standards is available in <a href="./rfc5741#section-2">Section&nbsp;2 of RFC 5741</a>.
---
>    Internet Standards is available in <a href="./rfc5741#section-2">Section 2 of RFC 5741</a>.
611c611
<    The entire <a href="./rfc2595#section-2.4">Section&nbsp;2.4 of RFC 2595</a> is replaced with the following
---
>    The entire <a href="./rfc2595#section-2.4">Section 2.4 of RFC 2595</a> is replaced with the following
618c618
<    The 3rd paragraph (and its subparagraphs) in <a href="./rfc3501#section-11.1">Section&nbsp;11.1 of RFC 3501</a>
---
>    The 3rd paragraph (and its subparagraphs) in <a href="./rfc3501#section-11.1">Section 11.1 of RFC 3501</a>
625c625
<    The 3rd paragraph (and its subparagraphs) in <a href="./rfc3207#section-4.1">Section&nbsp;4.1 of RFC 3207</a>
---
>    The 3rd paragraph (and its subparagraphs) in <a href="./rfc3207#section-4.1">Section 4.1 of RFC 3207</a>
640c640
<       described in <a href="./rfc5804#section-2.2.1.2">Section&nbsp;2.2.1.2 of RFC 5804</a>.
---
>       described in <a href="./rfc5804#section-2.2.1.2">Section 2.2.1.2 of RFC 5804</a>.

RFC 8649:

❯ diff test_rfcs/rfc8649.txt.OLD.html test_rfcs/rfc8649.txt.NEW.html
29c29
<    Standard; see <a href="./rfc7841#section-2">Section&nbsp;2 of RFC 7841</a>.
---
>    Standard; see <a href="./rfc7841#section-2">Section 2 of RFC 7841</a>.

[logan-stytch]

Just wanted to bump here again. Anything else needed to get this committed?

[jennifer-richards]

Just wanted to bump here again. Anything else needed to get this committed?

Thank you for the bump, and sorry for the delay- looking again.

[kesara]

LGTM. Thanks for the PR.

[jennifer-richards]

Ran this over all the rfcs (minus a few that had encoding issues my quick-and-dirty script didn't deal with). The   to change is common, especially for recent RFCs where it affects the boilerplate.

Ran across a case where a reference that spanned a newline that was split into two separate <a>...</a> links became a single link. That's an improvement.

Didn't find any alarming side effects so I'm happy with it.

[jennifer-richards]

Oops- spoke too soon. A line break in RFC 7925 is impacted badly. Startingn at line 1256 in the rendered HTML,

used.  The UTF-8 encoding of identities described in <a href="./rfc4279#section-5.1">Section&nbsp;5.1 of
   RFC 4279</a> aims to improve interoperability for those cases where the

becomes

   used.  The UTF-8 encoding of identities described in <a href="./rfc4279#section-5.1">Section 5.1 of RFC 4279</a> aims to improve interoperability for those cases where the

Since this is in a <pre> block, the newline loss results in a too-long line.

[jennifer-richards]

Sorry for the churn, but see comment re: significant linebreak changes in rfc7925 and possibly others

Edit to add: impacts quite a few. Scanned for changes in line counts and 364 show up. Spot checking, it looks like the same issue in all of them.

line-count-changes.txt.zip

[logan-stytch]

I think I've got the requested changes fixed, but let me know what you think. I also made a (local) utility to compare main's output to my branch in case you think it could be helpful to commit here.

[jennifer-richards]

Getting closer! There are still ~100 (108 by my not perfect method) that have line count changes, though. Example in rfc1122:

old

         4.2.2.9  Initial Sequence Number Selection: <a href="./rfc793#section-3.3">RFC-793 Section&nbsp;</a>
            <a href="#section-3.3">3.3</a>, page 27

vs new

         4.2.2.9  Initial Sequence Number Selection: <a href="./rfc793#section-3.3">RFC-793 Section&nbsp;            3.3</a>, page 27

line-count-changes.txt.zip

[jennifer-richards]

Thank you for the update! Is this ready for a re-check or do you have more changes coming?

[logan-stytch]

@jennifer-richards - I should probably do my own testing first, I've been pretty busy the last month and haven't had a chance to test thoroughly yet.

[logan-stytch]

Okay, I ran my tests comparing old vs new output and I think this is ready for another review!

[jennifer-richards]

Okay, I ran my tests comparing old vs new output and I think this is ready for another review!

Great, I'll give it a look tomorrow. Thank you!

[jennifer-richards]

This is moving forward - down from 108 to 87 line count changes I think. I'm attaching my list here in case you want to look at it. After the holidays, I'll look into this some more and talk with @kesara and others about how much we need to chase perfection here.

I've noticed one other change that I don't think is new with this version, but hadn't caught my eye before. In RFC 1002, in a ToC entry for QUESTION SECTION [bunch of spaces] 10, the SECTION [bunch of spaces] 10 was treated like a link to section 10 and was made into a link.

line-count-changes.txt.zip

[pjaitken]

Is there any chance of reviving this stalled PR?

I just re-reported the same issue: ietf-tools/datatracker#11116

[jennifer-richards]

Is there any chance of reviving this stalled PR?

Yes, and thanks for nudging it. It's waiting for a decision about balancing the improvement with regressions and got delayed by other priorities. Will push to get it some attention now.

[jennifer-richards]

(Finally) I think we should merge this. It's a net improvement. I'll file a new issue for the minor regression on ToC links.