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

Gregsdennis/external links #1568

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented Jan 5, 2025

What kind of change does this PR introduce?

editorial

Issue & Discussion References

Summary

Inline all of the links to external documents and remove the associated appendix.

  • Still a couple open comments on the issue to address
  • need to perform markdown cleanup

Does this PR introduce a breaking change?

No

@gregsdennis gregsdennis requested a review from a team January 5, 2025 04:00
@gregsdennis gregsdennis self-assigned this Jan 5, 2025
@gregsdennis gregsdennis added this to the stable-release milestone Jan 5, 2025
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Reference style links might be a good choice for this. Especially if we end up making the same reference in multiple places.

## Blah
Blah, blah, [JSON Pointer][RFC6901], blah.

### Again
Some other reference to [JSON Poiner][RFC6901] elsewhere in the spec.

<!-- Reference links can go in one place at the bottom -->
[RFC6901]: https://www.rfc-editor.org/info/rfc6901

@gregsdennis gregsdennis force-pushed the gregsdennis/external-links branch from 146285b to a3ba78a Compare January 10, 2025 04:57
@gregsdennis gregsdennis marked this pull request as ready for review January 10, 2025 19:40
@gregsdennis
Copy link
Member Author

Alright I think that's it for the primary changes of this PR. The only thing left is figuring out cross-document links.

@jdesrosiers I'm not sure whether you want to support that as part of this PR. Alternatives are we merge this in the broken state or it gets supported in another PR and this is rebased. I'm happy with either, really, but the latter seems more proper.

@gregsdennis gregsdennis requested a review from a team January 10, 2025 19:42
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Everything looks good so far.

The only thing left is figuring out cross-document links.

I don't think there's an easy way around this problem. I'm going to take another look, but if I don't find anything in a relatively short amount of time, I think we should merge this. It really bothers me to commit something that's breaking the build, but it might be all we can do right now.

@jdesrosiers
Copy link
Member

I just remembered a solution I had in mind that I want to write down so I don't forget again.

I think the best approach is to write links to the .md source, but convert the link to .html as part of the build to HTML. In that case, the right thing would be to make the links point to .md before merging this and then add the link conversion plugin later.

@jdesrosiers
Copy link
Member

Rewriting the links was easy and works out great. Not only does it make the link checker work, but it also allows links to work both in Github, not just when built to HTML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Link directly to targets
2 participants