feat: Add Debug derives to Slot, SlotGuard, and AppendAndCloseOnDrop#194
Conversation
jlizen
left a comment
There was a problem hiding this comment.
This is an improvement for sure, but I'm wondering if it could be written in a way that has a lossier debug impl in case T/T::Closed: !Debug. That shows whether things are open or closed.
|
|
||
| impl<T: Debug + CloseValue> Debug for SlotGuard<T> | ||
| where | ||
| T::Closed: Debug, |
There was a problem hiding this comment.
This is fine, but is there a reason we wouldn't want to have a custom debug impl for SlotI instead of just the #[derive(Debug)]?
That way (I think) we could have one that is T: Debug and includes the inner contents, or T: !Debug which just shows whether it is still open or not
| /// metrics.field_1 += 1; | ||
| /// } | ||
| /// ``` | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
Same feedback as with SlotI... this is an improvement, but it seems like we could provide a debug impl even if T: !Debug and T::Closed: !Debug?
| }) | ||
| } | ||
|
|
||
| fn extract_allowed_derives(attrs: &[Attribute]) -> Ts2 { |
There was a problem hiding this comment.
Snapshot test with this would be nice.
| let tokens = meta_list.tokens.to_string(); | ||
| for derive in &allowed { | ||
| if tokens.contains(derive) { | ||
| found.push(syn::Ident::new(derive, meta_list.span())); |
There was a problem hiding this comment.
The only difference between this one and struct is that we point errors to the derive tokens instead of the metrics macro call site. This seems better / any reason we can't just use the same function for both?
|
|
||
| impl Debug for Guard { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("Guard").finish() |
There was a problem hiding this comment.
Would be nice to show whether guard is open or not
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] |
| /// // When `metrics` is dropped, it will be closed and appended to the sink | ||
| /// # } | ||
| /// ``` | ||
| #[derive(Debug)] |
2b5f21d to
6ca7427
Compare
|
I did not opt for a opaque impl — you can't have both due to a lack of specialization and I think most users are fine with |
- Unify extract_allowed_derives function between enums and structs - Add UI test for Debug derives passthrough - Show open/closed state in Guard Debug impl
📬 Issue #, if available: Fixes #190
✍️ Description of changes: Add
derive(Debug)to Slot & friends. This also required updating the proc macro to pass throughDebugwhen present.🔏 By submitting this pull request