-
-
Notifications
You must be signed in to change notification settings - Fork 508
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
zmerp-staging #2625
base: master
Are you sure you want to change the base?
zmerp-staging #2625
Conversation
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.
This should really be handled in dbg_connection!
itself, not like this, the family of debug stuff might trip up a lot of other places too.
I can't find an easy way of doing this unfortunately. The macro takes a |
0ed9f86
to
ce9c7e4
Compare
Failure on warnings works |
|
||
if !messages.is_empty() { | ||
panic!("ci clippy produced warnings"); | ||
} |
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'm pretty sure there's also other compiler messages than diagnostics (you'd have to check the rustc docs tho), but we should only cause a ci failure when a diagnostic is actually emitted.
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 checked and there's actually some, but they're not very frequent. Either way the diagnostic message filtering should be moved to the main filter (or a second stacked filter_map), as that'll fully ensure that causes no issues.
if let WiredConnectionStatus::NotReady(m) = status { | ||
dbg_connection!("handshake_loop: Wired connection not ready: {m}"); | ||
#[cfg_attr(not(debug_assertions), allow(unused_variables))] | ||
if let WiredConnectionStatus::NotReady(s) = status { |
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.
Could make this an expect ig
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.
Right. All allows should be turned into expect actually.
But the moving the warning handling was more of a suggestion, if it doesn't work then it doesn't work. |
No description provided.