chore: use rand.Text and slog.DiscardHandler over intrernal implementation#773
Conversation
maciej-kisiel
left a comment
There was a problem hiding this comment.
Thanks for the contribution, I have one comment.
mcp/util.go
Outdated
| // TODO: once 1.24 is assured, just use crypto/rand. | ||
| const base32alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" | ||
|
|
||
| func randText() string { |
There was a problem hiding this comment.
Could we replace calls to this function with rand.Text() and remove it altogether?
There was a problem hiding this comment.
Thanks for the review, @maciej-kisiel. Addressed your review comment.
I did not remove randText method earlier, to minimize code change and also as ServerOptions.GetSessionID expects a func, and so anyway need to create anon func. And mcp/streamable.go, uses math/rand/v2, causing a conflict.
|
I did not remove randText method earlier, to minimize code change and ServerOptions.GetSessionID expects a func, and so anyway need to create anon func. Also mcp/streamable.go, uses math/rand/v2, causing a conflict and impacting readability. |
mcp/server.go
Outdated
|
|
||
| if opts.GetSessionID == nil { | ||
| opts.GetSessionID = randText | ||
| opts.GetSessionID = func() string { |
There was a problem hiding this comment.
Just assigning rand.Text won't work?
There was a problem hiding this comment.
No, it won't. SessionId expects func()string. I mentioned this in the response to your earlier review comment on why I reused the existing randText method earlier.
There was a problem hiding this comment.
rand.Text is func() string. Please do:
opts.GetSessionID = rand.Text
There was a problem hiding this comment.
ahaa, my bad. resolved.
|
Thanks for the contribution! |
Address TODOs in code requiring go 1.24 as minimum version
mcp/logging.go: use slog.DiscardHandler, over local implementation
mcp/util.go: use rand.Text, over local implementation
Fixes #772