-
Notifications
You must be signed in to change notification settings - Fork 26
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
tweak child_specs #47
Conversation
08ea561
to
11c6c11
Compare
11c6c11
to
d2d5e01
Compare
lib/nsq/connection.ex
Outdated
@@ -62,18 +63,15 @@ defmodule NSQ.Connection do | |||
# ------------------------------------------------------- # | |||
# Behaviour Implementation # | |||
# ------------------------------------------------------- # | |||
@spec start_link(pid, host_with_port, NSQ.Config.t(), String.t(), String.t(), pid, list) :: | |||
@spec start_link(pid | host_with_port | NSQ.Config.t() | String.t() | String.t() | pid) :: |
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.
This spec says you receive a single argument that is any one of these. However, you actually receive a list with each of these positionally located in that list. The correct spec would be the following:
@spec start_link(pid | host_with_port | NSQ.Config.t() | String.t() | String.t() | pid) :: | |
@spec start_link([pid, host_with_port, NSQ.Config.t(), String.t(), String.t(), pid]) :: |
It looks like you have the same problem in a couple other places that you should fix as well.
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.
Hey, good one!
I was scratching my head on this a bit.
If I do this (at a different function but same at all) I get this error when I compile:
unexpected list in typespec: [binary(), NSQ.Config.t()]
I couldn't really find any working examples of this, or could not make it work.
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 think the simplest here is to use tuples instead of the lists.
If you know how to make the list work let me know!
Cheers
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.
Yea, I think tuple is probably what's appropriate here.
According to the docs this setup is less error prone, since we don't define the maps anymore.
To support the tuple child spec I changed some
start_link
s.I also noticed a missing
use Genserver
which could lead to unexpected things I think.