-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix theme-checker failure #61181
Fix theme-checker failure #61181
Conversation
You should probably add a regression test here if possible |
|
Try writing a test on this function? |
It doesn't even need to assert anything. TDD dictates you need a failing test case to change code. Panicking is failing the test |
Or |
Are you referring to a system level test here? That might be more difficult. |
I was speaking at a system level, indeed. :) I'll add a function test instead. |
I added a test but like I said, I don't think this is the best way. I guess it'll work for now... |
It seems the check |
I think it's a valid request so I'll remove them. |
Removed them! |
r? @Manishearth |
I don't really have time to review this rn. |
Let's try someone else then! cc @rust-lang/rustdoc |
r? @ollie27 |
r? @kinnison |
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.
A few comments, but otherwise LGTM. I'm not sure if I'm able to approve properly on this codebase but you have a 👍 from me modulo the query about panicing.
bdabbfc
to
d723b4b
Compare
@kinnison Updated! |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
Modulo the need for a borrow in that rewritten test, which I've just realised is in a non-public function so your comment about the guard in the public function meaning it won't be necessary anyway holds, 👍
d723b4b
to
640bdbd
Compare
Removed the last commit then. Thanks for your review! @bors: r=kinnison |
📌 Commit 640bdbd has been approved by |
…=kinnison Fix theme-checker failure Fixes rust-lang#61145. I didn't find a way to check it without strongly depending on the output... Is there a way to check if a program fails without checking its output? r? @QuietMisdreavus
…=kinnison Fix theme-checker failure Fixes rust-lang#61145. I didn't find a way to check it without strongly depending on the output... Is there a way to check if a program fails without checking its output? r? @QuietMisdreavus
…=kinnison Fix theme-checker failure Fixes rust-lang#61145. I didn't find a way to check it without strongly depending on the output... Is there a way to check if a program fails without checking its output? r? @QuietMisdreavus
Rollup of 9 pull requests Successful merges: - #60971 (Add DocFS layer to rustdoc) - #61146 (SliceConcatExt::connect defaults to calling join) - #61181 (Fix theme-checker failure) - #61267 (rustc-book: Update the rustc/clang compatibility table for xLTO.) - #61270 (Remove warnings about incr. comp. generating less debugging output.) - #61681 (Changed the error message to more clearly explain what is allowed) - #61984 (More NodeId pruning) - #62016 (Add test for issue-27697) - #62019 (Remove needless lifetimes) Failed merges: r? @ghost
Fixes #61145.
I didn't find a way to check it without strongly depending on the output... Is there a way to check if a program fails without checking its output?
r? @QuietMisdreavus