-
-
Notifications
You must be signed in to change notification settings - Fork 885
fix: relative links in gleam toml #5076
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?
fix: relative links in gleam toml #5076
Conversation
Signed-off-by: Courtcircuits <[email protected]>
Signed-off-by: Courtcircuits <[email protected]>
Signed-off-by: Courtcircuits <[email protected]>
ff255f8 to
0f45d8f
Compare
Signed-off-by: Courtcircuits <[email protected]>
| .links | ||
| .iter() | ||
| .map(|l| (l.title.as_str(), l.href.clone())) | ||
| .filter(|(_, href)| !href.clone().is_internal()) |
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.
Put the filter before the mapping and remove the clone please 🙏
| pub title: String, | ||
| #[serde(with = "uri_serde")] | ||
| pub href: Uri, | ||
| pub href: Href, |
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 Href type here looks like it'll permit data that's not valid URLs. How about instead we use the URL type from the url crate? It's already an indirect dep, so we can add it and use it without adding a new dep.
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 read through url crate documentation and it seems like it needs a "base url" to validate a relative path, which is the goal of this PR.
Many contexts allow URL references that can be relative to a base URL:
<link rel="stylesheet" href="../main.css">Since parsed URLs are absolute, giving a base is required for parsing relative URLs:
use url::{Url, ParseError};
assert!(Url::parse("../main.css") == Err(ParseError::RelativeUrlWithoutBase))Use the join method on an Url to use it as a base URL:
use url::Url;
let this_document = Url::parse("http://servo.github.io/rust-url/url/index.html")?;
let css_url = this_document.join("../main.css")?;
assert_eq!(css_url.as_str(), "http://servo.github.io/rust-url/main.css");I think the main problem with my implementation is that recursive paths are valid, which should only be the case if the "base url" provides a path (e.g. http://myhexdoc.com/foo/bar). What I could do is to check if the user provided an "Home Page" within the link list, if so it will use that home page link as the base url. If not then I could fall back to a strict generic base url without path like http://foo.com/.
What do you think about that strategy ?
What has been done ?
This PR aims to resolve #4772. I made the gleam.toml parsing less strict by allowing the usage of relative links within the documentation.
For example this manifest is now correct :
The generated doc looks like this :
As you can see this doc contains a relative link to "./internal" which wasn't possible previously.
And the hex doc looks like this :
You can find the hexdoc page here.
The link called "Website internal" is not present since it would be strange for hex to contain relative links.