Skip to content
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

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

docmerlin
Copy link
Contributor

No description provided.

Copy link
Contributor

@davidby-influx davidby-influx left a 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) {
Copy link
Contributor

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 {
Copy link
Contributor

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?

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 + "\"")
Copy link
Contributor

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.

Copy link
Contributor

@davidby-influx davidby-influx left a 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)
Copy link
Contributor

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.

@docmerlin docmerlin merged commit 83bf151 into master Jun 17, 2022
@docmerlin docmerlin deleted the usego1.18.0 branch June 17, 2022 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants