-
Notifications
You must be signed in to change notification settings - Fork 488
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
chore: update go to 1.18 and refactor to take advantage of some of it… #2687
Conversation
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.
A few questions, nothing major.
circularqueue.go
Outdated
} | ||
} | ||
|
||
// Enqueue adds an item to the queue. | ||
func (q *timeMessageCircularQueue) Enqueue(v timeMessage) { | ||
func Enqueue[T any](q *CircularQueue[T], v T) { |
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 this be a receiver function, or is there some generics issue with that?
circularqueue.go
Outdated
q.head = 0 | ||
q.tail = 0 | ||
} | ||
return | ||
} | ||
|
||
// Peek peeks i ahead of the current head of queue. It should be used in conjunction with .Len() to prevent a panic. | ||
func (q *timeMessageCircularQueue) Peek(i int) timeMessage { | ||
if i < 0 || i >= q.l { | ||
func Peek[x any](q *CircularQueue[x], i int) x { |
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 this be a receiver function?
services/load/service.go
Outdated
return errors.New("expected id to be topicID/handlerID") | ||
topicID, handlerID, found := strings.Cut(id, "/") | ||
if !found { | ||
return errors.New("expected id to be topicID/handlerID but it was: \"" + id + "\"") |
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.
Perhaps use fmt.Errorf
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.
LGTM
One non-blocking suggestion.
@@ -603,7 +603,7 @@ func (s *Service) removeHandlers() error { | |||
for _, id := range diff(s.handlers, loadedHandlers) { | |||
topicID, handlerID, found := strings.Cut(id, "/") | |||
if !found { | |||
return errors.New("expected id to be topicID/handlerID but it was: \"" + id + "\"") | |||
return fmt.Errorf("expected id to be topicID/handlerID but it was: \"%s\"", id) |
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.
non-blocking - I like %q
over \"%s\"
because it escapes bad characters and looks cleaner in the formatting string.
No description provided.