-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Derive Debug
for SessionStateBuilder
, adding Debug
requirements to fields
#12632
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
Conversation
… informs this is unnecessary This reverts commit f69d73c.
Needs the |
Debug
for SessionStateBuilder
, adding Debug
requirements to fieldsDebug
for SessionStateBuilder
, adding Debug
requirements to fields
Feel like not adding the derive Debug for |
… keep consistent Debug field order with `SessionState`
… `CatalogProviderList` requires `Debug`
Now the impl Debug for |
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.
Thank you @AnthonyZhOon -- this looks like a great improvement to me. I agree that the actual utility of the Debug
impl is debatable, but at least now it is possible
impl Debug for InformationSchemaConfig { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
f.debug_struct("InformationSchemaConfig") | ||
// TODO it would be great to print the catalog list here |
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.
🎉
@@ -177,23 +177,23 @@ impl Debug for SessionState { | |||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |||
f.debug_struct("SessionState") | |||
.field("session_id", &self.session_id) | |||
.field("analyzer", &"...") | |||
.field("config", &self.config) |
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 wonder if we could simply #derive(Debug)
for SessionState now?
And perhaps we could provide a impl Display for SessionState
that had a more readable format (like that shows session_id and deviation from default configuration, for example 🤔 )
Thanks again @AnthonyZhOon |
Which issue does this PR close?
Progress on #12555
Rationale for this change
To make configuration easier to use by providing debug output.
What changes are included in this PR?
Debug
forCatalogProvider
,CatalogProviderList
,UrlTableFactory
.Debug
forExprPlanner
,QueryPlanner
.Debug
forTableFunctionImpl
Debug
forSerializerRegistry
Debug
forFunctionFactory
#derive
debug on necessary classesDebug
onSessionStateBuilder
by deriving.Are these changes tested?
By the compiler and CI.
Are there any user-facing changes?
The additional
Debug
requirement is a user-facing API change on the traits affected.