-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add support for fmt-debug #1798
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1798 +/- ##
=======================================
Coverage 87.40% 87.40%
=======================================
Files 16 16
Lines 5995 5995
=======================================
Hits 5240 5240
Misses 755 755 ☔ View full report in Codecov by Sentry. |
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.
Wow, had no idea, thanks! One small change and I'll approve this.
@@ -1076,7 +1076,17 @@ enum Trait { | |||
|
|||
impl ToTokens for Trait { | |||
fn to_tokens(&self, tokens: &mut TokenStream) { | |||
let ident = Ident::new(&format!("{:?}", self), Span::call_site()); | |||
let s = match self { |
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.
Can you add a comment here explaining why this verbosity is necessary and just using Debug isn't sufficient?
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.
Done. PTAL
The unstable feature fmt-debug [1] allows to influence `derive(Debug)` implementations, for example by making it return an empty string. As the standard library also does not make any guarantees on the outputs of `derive(Debug)` , this change does replace the reliance on it by an explicit conversion to the variant name.
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.
Awesome, thanks! I pushed a minor edit to this to make it consistent with our commit message style (including squashing everything into a single commit), and it should merge as soon as it goes through CI and the merge queue.
The unstable feature fmt-debug allows to influence
derive(Debug)
implementations, for example by making it return an empty string. As the standard library also does not make any guarantees on the outputs ofderive(Debug)
, this change does replace the reliance on it by an explicit conversion to the variant name.