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

Upgrade to Gun2.0 #58

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Upgrade to Gun2.0 #58

merged 6 commits into from
Mar 21, 2023

Conversation

belltoy
Copy link
Contributor

@belltoy belltoy commented Mar 16, 2023

Based on #55.

And fixes some var warnings in test suites.

@zhongwencool
Copy link
Owner

I don't have time to review this right now :(.
please help to review this @enjolras1205

@belltoy
Copy link
Contributor Author

belltoy commented Mar 16, 2023

Gun 2.0 is finally out so the original PR #55 can upgrade to it directly. Or I can do this based on #55 if @enjolras1205 don't mind.

@zhongwencool Would you merge this and bump a new version on hex.pm? After reviews, off course.

@belltoy belltoy changed the title Upgrade to Gun2.0 WIP: Upgrade to Gun2.0 Mar 16, 2023
@belltoy
Copy link
Contributor Author

belltoy commented Mar 16, 2023

The original PR introduced some breaking changes. So I will add some changelogs if those changes are acceptable.

@belltoy
Copy link
Contributor Author

belltoy commented Mar 18, 2023

The changes from #55 seem good to me.

As per the discussions in #28, I upgrade eetcd version to 0.4.0 with a migration note attached to the README.

Fixes the outdated docs for the eetcd:open/5,6.

@belltoy belltoy changed the title WIP: Upgrade to Gun2.0 Upgrade to Gun2.0 Mar 18, 2023
src/eetcd.erl Outdated
open(Name, Hosts, Transport, TransportOpts) ->
open(Name, Hosts, [], Transport, TransportOpts).
open(Name, Hosts, Transport, TcpOpts, TlsOpts) when is_atom(Transport) ->
open(Name, Hosts, [], Transport, TcpOpts, TlsOpts).
Copy link
Contributor

@gilbertwong96 gilbertwong96 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combining TcpOpts and TlsOpts into a single function seems somewhat unusual.

  • TcpOpts is associated with the transport tcp
  • TlsOpts is associated with the transport ssl or tls

I think it is better to put Transport and TransportOpts into the eetcd Options.

-spec open(Name, Hosts, Options)  -> {ok, pid()} | {error, any()} when
    Name :: name(),
    Hosts :: [string()],
    Options :: [ {mode, connect_all | random} |
                 {name, string()} |
                 {password, string()}  |
                 {retry, non_neg_integer()} |
                 {retry_timeout, pos_integer()} |
                 {connect_timeout, timeout()} 
                 {transport, tcp | ssl | tls} |
                 {tcp_opts, [gen_tcp:connect_option()]} |
                 {tls_opts, [ssl:tls_client_option()]}
               ].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, this pull request will introduce a breaking change. I believe we can offer a simpler API in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I have tried to make this change, and it looks good.

README.md Outdated Show resolved Hide resolved
belltoy added 2 commits March 19, 2023 12:32
These changes break the original API style, replace it with a simpler style: `eetcd:open(Name, Endopoints, Options)`.
@zhongwencool zhongwencool merged commit 0cdd103 into zhongwencool:master Mar 21, 2023
@zhongwencool
Copy link
Owner

Thanks a lot!

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.

4 participants