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.

Sign up for GitHub

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

Jump to bottom

Add lint for copyright headers to 'tidy' tool #57520

Merged
merged 4 commits into from
Jan 17, 2019
Merged

Add lint for copyright headers to 'tidy' tool #57520

merged 4 commits into from
Jan 17, 2019

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Jan 11, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg
Copy link
Contributor Author

alexreg commented Jan 11, 2019

Looks to be working. I’ll fix those files shortly.

@alexreg alexreg force-pushed the tidy-copyright-lint branch from dd58319 to cade87c Compare January 11, 2019 19:29
@rust-highfive

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

Looks like Travis failed but this looks good to me in general - it might be worth including a check for "Rust" in the copyright line?

@alexreg alexreg force-pushed the tidy-copyright-lint branch from cade87c to 869ea16 Compare January 11, 2019 20:43
@alexreg
Copy link
Contributor Author

alexreg commented Jan 11, 2019

Looks like Travis failed but this looks good to me in general - it might be worth including a check for "Rust" in the copyright line?

Yep, I actually went further than this and made a regex for the copyright line, but let me know if that's overkill...

@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the tidy-copyright-lint branch from 869ea16 to 8faeb69 Compare January 11, 2019 23:05
@Mark-Simulacrum
Copy link
Member

I think the regex is a bit overkill - especially because as far as I can tell it can be replaced with ~3 contains nearly equivalently. But if that's not actually the case, then let's put it in a lazy_static and call it good.

@alexreg alexreg force-pushed the tidy-copyright-lint branch from 8faeb69 to 1080464 Compare January 12, 2019 02:10
@alexreg
Copy link
Contributor Author

alexreg commented Jan 12, 2019

Yeah, I thought originally I would need a more complicated regex to distinguish between Rust Developers and other sorts of copyright notices, but probably not. Let's see if this works...

@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the tidy-copyright-lint branch from 1080464 to 0ec1266 Compare January 12, 2019 03:25
@alexreg
Copy link
Contributor Author

alexreg commented Jan 12, 2019

@Mark-Simulacrum Looking good now. (Tidy checks would have failed this far into the tests.) r+ if you're happy with things?

@Mark-Simulacrum
Copy link
Member

r=me with notes fixed.

Thanks for taking this on!

Let's also @bors p=1 to hopefully help avoid some rebase pain.

@alexreg alexreg force-pushed the tidy-copyright-lint branch from 0ec1266 to cbd0d86 Compare January 12, 2019 17:09
@alexreg
Copy link
Contributor Author

alexreg commented Jan 12, 2019

@Mark-Simulacrum LGTM?

@rust-lang rust-lang deleted a comment from bors Jan 12, 2019
@rust-lang rust-lang deleted a comment from bors Jan 12, 2019
@rust-lang rust-lang deleted a comment from bors Jan 12, 2019
@rust-lang rust-lang deleted a comment from bors Jan 12, 2019
@rust-lang rust-lang deleted a comment from bors Jan 12, 2019
@bors
Copy link
Contributor

bors commented Jan 12, 2019

@alexreg: 🔑 Insufficient privileges: Not in reviewers

@alexreg
Copy link
Contributor Author

alexreg commented Jan 12, 2019

@Mark-Simulacrum Did your above r=me have any meaning to Bors or just a reminder?

Also, @nikomatsakis, was I added to the reviewers list yet? I tried to do @bors r=@Mark-Simulacrum p=1...

@bors
Copy link
Contributor

bors commented Jan 12, 2019

@alexreg: 🔑 Insufficient privileges: not in try users

@bors

This comment has been minimized.

Sign in to view
@alexreg
Copy link
Contributor Author

alexreg commented Jan 12, 2019

(Ugh, @bors should really ignore commands within backticks.)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 13, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019

@Mark-Simulacrum Fair enough. Do you have merge rights on the Book? It turns out there are some copyright notices in the code there blocking it. rust-lang/book#1765 fixes it, so maybe you could approve that if possible (once CI passes)...

@Mark-Simulacrum
Copy link
Member

Merged book PR.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019

@Mark-Simulacrum Okay, everything should be good now. Just r+ when Travis goes green please. :-)

@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 13, 2019

☔ The latest upstream changes (presumably #57568) made this pull request unmergeable. Please resolve the merge conflicts.

@alexreg alexreg force-pushed the tidy-copyright-lint branch from 114ba30 to 5a9693b Compare January 13, 2019 15:43
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019 •
edited
Loading

@Mark-Simulacrum @Centril I don't get the above errors when running locally... what's going on?

@alexreg alexreg force-pushed the tidy-copyright-lint branch from 5a9693b to 130d7d0 Compare January 13, 2019 19:47
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg
Copy link
Contributor Author

alexreg commented Jan 13, 2019

@Mark-Simulacrum @rust-lang/docs Could one of you kindly fix the links in the book, per above, so we can get this merged? Thanks.

@carols10cents
Copy link
Member

@alexreg The broken links in the book should be fixed now: rust-lang/book@0e9061c

@alexreg
Copy link
Contributor Author

alexreg commented Jan 16, 2019

@carols10cents Thanks a lot. I'll go ahead and update the submodule in this PR then. Appreciate you working with us despite that surprising merge to your repo over the weekend!

@alexreg alexreg force-pushed the tidy-copyright-lint branch from 130d7d0 to 4d18023 Compare January 16, 2019 03:59
@alexreg
Copy link
Contributor Author

alexreg commented Jan 16, 2019

@Mark-Simulacrum @Centril Okay, this should be ready to merge now... when CI passes, at least.

@Centril
Copy link
Contributor

Centril commented Jan 17, 2019

@bors r=Mark-Simulacrum p=10

@bors
Copy link
Contributor

bors commented Jan 17, 2019

📌 Commit 4d18023 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2019
@bors
Copy link
Contributor

bors commented Jan 17, 2019

⌛ Testing commit 4d18023 with merge 6599946...

bors added a commit that referenced this pull request Jan 17, 2019
@bors
Auto merge of #57520 - alexreg:tidy-copyright-lint, r=Mark-Simulacrum
6599946 
Add lint for copyright headers to 'tidy' tool

r? @Mark-Simulacrum

CC @Centril
@bors bors mentioned this pull request Jan 17, 2019
Closed
@bors
Copy link
Contributor

bors commented Jan 17, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Mark-Simulacrum
Pushing 6599946 to master...

@bors bors merged commit 4d18023 into rust-lang:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexreg @rust-highfive @Mark-Simulacrum @bors @Centril @carols10cents