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

Noobish1/improvements #60

Merged
merged 35 commits into from
Jul 6, 2016
Merged

Noobish1/improvements #60

merged 35 commits into from
Jul 6, 2016

Conversation

hamchapman
Copy link
Contributor

@hamchapman hamchapman commented Jun 21, 2016

Based on #57

I just fixed up some stuff aesthetically and fixed (most) of the tests. 2 still failing but easily fixable. I just stopped because I'm not sure that the PusherClientOptions, as it is, is the nicest way for a developer to interact with the library in terms of setting options. It's certainly far Swiftier but if, for example, you just want to set encrypted to false, then you have to do something like this:

let options = PusherClientOptions(
    authMethod: .NoMethod, 
    attemptToReturnJSONObject: true, 
    autoReconnect: true, 
    host: PusherHost.Cluster("mt1"), 
    port: nil, 
    encrypted: false
)

which seems sub optimal.

@Noobish1 WDYT?

-- Update --

You don't have to do that ^^ 👍

@Noobish1
Copy link
Contributor

are you saying the options are bad because you have to pass in multiple options? because you don't have to do that because most of those things have default values.

@hamchapman
Copy link
Contributor Author

hamchapman commented Jun 21, 2016

Replying on my phone so I can't check but I thought you still had to specify the values for parameters that come before a later one, i.e. can you do:

let options = PusherClientOptions(encrypted: false)

On 21 Jun 2016, 17:01 +0100, Blair McArthurnotifications@github.com, wrote:

are you saying the options are bad because you have to pass in multiple options? because you don't have to do that because most of those things have default values.


You are receiving this because you authored the thread.
Reply to this email directly,view it on GitHub(#60 (comment)), ormute the thread(https://github.com/notifications/unsubscribe/ACXEjio31omfS2JBCl7cN_sBJwXhsIbPks5qOArggaJpZM4I64zr).

@Noobish1
Copy link
Contributor

yup you only have to specify what you need or the stuff that doesn't have default values (which is nothing in this case). You can do let options = PusherClientOptions() even.

@Noobish1
Copy link
Contributor

Noobish1 commented Jun 21, 2016

That's actually the default value for the options in the Pusher init method.

@Noobish1
Copy link
Contributor

Noobish1 commented Jun 23, 2016

@hamchapman: Got another possible tweak to the auth builder: Noobish1/pusher-websocket-swift@1ca0df3...Noobish1:feature/pass-all-params-to-authrequestcustomizer

Could also make the requestForAuthEndpoint function public so clients could use that to get the request without doing it themselves.

Biggest benefit is that the client doesn't have to pass their authEndpoint and set a builder, they just pass the builder as they are building the whole request anyway.

@hamchapman
Copy link
Contributor Author

Okay this is now in the state I'd like it to be in (bar potentially some small README updates).

@Noobish1 - happy with it as it is?

I'll push out a 2.0.0 release very soon if you are.

Ideally I'd wait for Carthage/Carthage#1373 to get merged as then I can be lazy and make travis upload the built and zipped PusherSwift.framework.zip for Carthage users, but it's not a deal-breaker

@Noobish1
Copy link
Contributor

Looks good to me 👍

@hamchapman hamchapman merged commit ad55e99 into master Jul 6, 2016
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