Allow logging base64-encoded queries, instead of replacing bad chars. #1667
Conversation
|
Matching dolt PR: dolthub/dolt#5615 |
server/handler.go
Outdated
| sel ServerEventListener | ||
| } | ||
|
|
||
| var _ mysql.Handler = (*Handler)(nil) | ||
|
|
||
| // NewHandler creates a new Handler given a SQLe engine. | ||
| func NewHandler(e *sqle.Engine, sm *SessionManager, rt time.Duration, disableMultiStmts bool, maxLoggedQueryLen int, listener ServerEventListener) *Handler { | ||
| func NewHandler(e *sqle.Engine, sm *SessionManager, rt time.Duration, disableMultiStmts bool, maxLoggedQueryLen int, encodeLoggedQuery bool, listener ServerEventListener) *Handler { |
There was a problem hiding this comment.
This constructor is just crying out for a struct instead of all these separate params. Want to take another pass and add one?
There was a problem hiding this comment.
But we're just assigning those values to a struct... And if we replace the parameters with a struct, then we'll need a constructor for that struct (and that constructor will look suspiciously similar to this constructor).
If the method wasn't a constructor, I would agree, it's better to use a struct than so many parameters. But as it stands, we gain nothing by adding another struct here.
I'm inclined to keep the constructor as-is. Or if we're not happy with so many params, we can get rid of the constructor entirely. Thoughts?
There was a problem hiding this comment.
The reason I'm unhappy with the number of parameters is that it becomes difficult at the call site to know what each of the many very similar parameters all mean, especially when they are booleans.
You could simply get rid of the constructor and construct the Handler struct directly (with named fields) at each call site. That would probably be better.
There was a problem hiding this comment.
Completely agree. Updated.
No description provided.