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

Detect relative urls in tidy check #43632

Merged
merged 2 commits into from
Aug 11, 2017

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Aug 3, 2017

This came up in #43631: there can be long relative urls in Markdown comments, that do not start with http:// or https://, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with ../.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

Code looks good to me. @rust-lang/docs, can you think of any accidental ignores we'd add by doing this? I'd imagine this is fine...

@GuillaumeGomez
Copy link
Member

It seems good for me.

@@ -79,11 +79,11 @@ fn line_is_url(line: &str) -> bool {
=> state = EXP_URL,

(EXP_LINK_LABEL_OR_URL, w)
if w.starts_with("http://") || w.starts_with("https://")
if w.starts_with("http://") || w.starts_with("https://") || w.starts_with("../")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so this change will permit long lines anywhere that there is a word beginning with ../. I'm not sure if this is what we want. Maybe it is, or maybe it's too accepting.

On the other hand, if we only keep the second change, below, then the [deref-coercions]: link in the relevant PR would put us into the EXP_URL state, and there we can accept ../, where the context is a bit less ambiguous.

I could go either way, just saying that maybe we don't want this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is currently one line in the codebase which would be affected:

$ git grep -E '\s?//(/|!)?\s\.\./'
src/libstd/net/tcp.rs:    /// ../../std/net/trait.ToSocketAddrs.html#tymethod.to_socket_addrs

It is part of a Markdown link.

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favour of removing (though I think it's just the first word on a line, rather than any).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Yellow is indeed a nice color for a bikeshed.
@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

r? @Mark-Simulacrum - @nikomatsakis is on vacation until Aug 15

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 9, 2017

📌 Commit 608863d has been approved by Mark-Simulacrum

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
bors added a commit that referenced this pull request Aug 11, 2017
MaloJaffre added a commit to MaloJaffre/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
@bors bors merged commit 608863d into rust-lang:master Aug 11, 2017
@ruuda ruuda deleted the allow-long-relative-urls branch August 11, 2017 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants