Don't exit when a connection fails and adds Paddle.reconnect/1

[shamanime]

This PR is a complement to #20.

It still needs improvements but the code works.

When starting Paddle, if the connection fails it will hold :not_connected as the state and will reply {:error, :not_connected} for messages it receives.

Paddle.reconnect/1 was added, it will reconnect using the connection options received as argument or fetch the options from the config.

[minijackson]

This looks great! Thank you so much!

Nitpicks:

• It could be nice to know why the connection would fail. How about the do_connect returns the Reason from :eldap.open (as the init and reconnect would, then), but keep the :not_connected as the GenServer state?
• I think some specs needs to be updated. I don't mind doing it, the :eldap doc doesn't seem to enumerate the different failures. I'll look at the code but if I remember correctly, it's a combination of LDAP specific errors and :gen_tcp's connect errors

Great job! I think that puts us closer to supporting multiple LDAP servers and multiple connections (#4).

[shamanime]

Thanks! Yeah returning the reason seems the best approach. I’ll do my best to update the specs but this is my first time doing it for OSS and I’m not very familiar. Will be a great chance for me to read more and get to know it better. I’m preparing to move out early next week. I may be able to get back to this around Wed/Thursday.

…

On 26 Jul 2018, at 1:52 PM, Rémi NICOLE ***@***.***> wrote: This looks great! Thank you so much! Nitpicks: It could be nice to know why the connection would fail. How about the do_connect returns the Reason from :eldap.open (as the init and reconnect would, then), but keep the :not_connected as the GenServer state? I think some specs needs to be updated. I don't mind doing it, the :eldap doc doesn't seem to enumerate the different failures. I'll look at the code but if I remember correctly, it's a combination of LDAP specific errors and :gen_tcp's connect errors Great job! I think that puts us closer to supporting multiple LDAP servers and multiple connections (#4). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[minijackson]

This should help with the specs

[shamanime]

Had a little time to improve the state. What do you say?

Now just need to work on the specs. Will give your link a good read next week!

[minijackson]

I re-read :eldap's code and apparently, :eldap.open does not return any meaningful error x) But let's keep it that way in case it changes (and I hope it will).

I see you're keeping the error around and reply that to subsequent calls. That could be useful, but I'm afraid it could give the wrong idea, and make people think it tries to reconnect each time :-/

My initial thought was that only init and reconnect would return the error from :eldap.open and the rest would just return {:error, :not_connected}

What's your take on that? Am I missing something?

[shamanime]

No, you're not missing anything. What you said makes sense.

I think we should be good now. I may have overlooked the specs but seems like the only needed change was the spec for ldap_conn, which now holds a pid or {:not_connected, reason}.

[minijackson]

Awesome!

Could do you just convert :eldap.open's error to a String (probably inside do_connect) and adapt ldap_conn's type accordingly? After that, it will be perfect for merging.

[shamanime]

Done!

[minijackson]

Perfect! Many thanks

[shamanime]

Thank YOU for working so patiently with me on this!