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

tidy: exempt URLs from the line length restriction #39790

Merged
merged 2 commits into from
Feb 15, 2017

Conversation

zackw
Copy link
Contributor

@zackw zackw commented Feb 13, 2017

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle. Therefore, comment
lines consisting solely of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( [foo](http://...) notation ) are
not exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the regex and lazy_static crates
to the tidy utility. This appears to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle.  Therefore, comment
lines consisting _solely_ of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( `[foo](http://...)` notation ) are
_not_ exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the `regex` and `lazy_static` crates
to the tidy utility.  This _appears_ to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@steveklabnik
Copy link
Member

This LGTM, but yeah, we're just getting to the place where we can use dependencies in places like this, see #39728 and #39633

/cc @rust-lang/compiler

@alexcrichton
Copy link
Member

We don't really need lazy-static/regex for this, can we just ignore long lines with "http://" and "https://" ?

@steveklabnik
Copy link
Member

Might need ../ too, maybe. Or we can cross that bridge if and when we come to it.

@zackw
Copy link
Contributor Author

zackw commented Feb 13, 2017

We don't really need lazy-static/regex for this, can we just ignore long lines with "http://" and "https://" ?

Well, that's essentially what I meant by a "hand-written parser", but I do think we should be a little more picky than just line.contains("http://") || line.contains("https://"). I got tripped up earlier by how permissive the parser for // ignore-tidy-foo is; currently style.rs is exempt from all of its own checks because contents.contains("ignore-tidy-whatever") matches itself!

... So anyway I wrote something, but it may not be the way someone 100% fluent in the language would write it, please tell me what you think.

@alexcrichton
Copy link
Member

@bors: r+

Yeah this wasn't really written to be bulletproof, just catch some common mistakes.

@bors
Copy link
Contributor

bors commented Feb 13, 2017

📌 Commit ff4758c has been approved by alexcrichton

@zackw
Copy link
Contributor Author

zackw commented Feb 13, 2017

@bors: retry

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
…lexcrichton

tidy: exempt URLs from the line length restriction

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle.  Therefore, comment
lines consisting _solely_ of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( `[foo](http://...)` notation ) are
_not_ exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the `regex` and `lazy_static` crates
to the tidy utility.  This _appears_ to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
…lexcrichton

tidy: exempt URLs from the line length restriction

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle.  Therefore, comment
lines consisting _solely_ of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( `[foo](http://...)` notation ) are
_not_ exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the `regex` and `lazy_static` crates
to the tidy utility.  This _appears_ to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
…lexcrichton

tidy: exempt URLs from the line length restriction

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle.  Therefore, comment
lines consisting _solely_ of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( `[foo](http://...)` notation ) are
_not_ exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the `regex` and `lazy_static` crates
to the tidy utility.  This _appears_ to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
…lexcrichton

tidy: exempt URLs from the line length restriction

The length of a URL is usually not under our control, and Markdown
provides no way to split a URL in the middle.  Therefore, comment
lines consisting _solely_ of a URL (possibly with a Markdown link
label in front) should be exempt from the line-length restriction.

Inline hyperlink destinations ( `[foo](http://...)` notation ) are
_not_ exempt, because it is my arrogant opinion that long lines of
that type make the source text illegible.

The patch adds dependencies on the `regex` and `lazy_static` crates
to the tidy utility.  This _appears_ to Just Work, but if you would
rather not have that dependency I am willing to provide a hand-written
parser instead.
bors added a commit that referenced this pull request Feb 14, 2017
Rollup of 8 pull requests

- Successful merges: #39659, #39730, #39754, #39772, #39785, #39788, #39790, #39813
- Failed merges:
@bors bors merged commit ff4758c into rust-lang:master Feb 15, 2017
@zackw zackw deleted the tidy-linelen-exempt-urls branch February 16, 2017 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants