Ensure configuration exists

[hovsater]

Currently, if important parts of the configuration is missing such as
key, secret or app_id the client will blow up when trying to reference
any of the attributes. An example of this would be executing a #get
request and building the proper signature. Since the attributes can be
nil we would get an error such as: "Can't convert nil to string".

I'm not super familiar with the library yet, so I may have overlooked certain aspects of the library. If I missed any methods that should be covered by with_ensure_configuration feel free to call it out. I'd be happy to adjust the pull request accordingly.

[zimbatm]

+1 for implementing something.

I don't think it's necessary to wrap the calls inside of a block. You could also have an ensure_config! method that raises if the config is missing an just prefix all the methods with that. I'm happy to merge it as is though.

[hovsater]

Hey @zimbatm. Good call! I think adding ensure_config! is a better approach. Let me update my pull request before merging.

[zimbatm]

Sweet. Ping me when it's done and I'll merge and make a release

[mdpye]

Rather than checking on each operation, would it not be better to either:

• Throw from the client initializer when called: you shouldn't be creating a client, or using the default client before setting the config.
• Remove the use of globals and the default client. Pass config in to the client constructor.

I realise both are backwards incompatible (the first more subtly than the second), but (the latter particularly) is also just better design, and follows much more closely the pattern of the other OO language libraries we have.

The global config and default client feel very much like a hangover from the original "all global, all the time" implementation.

[hovsater]

Hey, @mdpye! Some great insights for sure. I think the second option is the most viable one. Currently, there is also some work to be done with separation of concerns when it comes to the client and the config. I think extracting the configuration into its own Configuration object could be a good idea and then, as you said, injecting the config into the client.

This will be inline with how other popular libraries solve this problem.

What are your thoughts? I can work on a pull request for this, dropping the globals all together and introducing the Configuration object. This will be breaking changes though.

[zimbatm]

We where getting pretty close to a useful change which didn't break backward-compatibility.

@KevinSjoberg if you feel like investing more energy into the project I'm not against the bigger OOP refactor but it should also have proper notification errors when the old globals are used. We also need to fix our docs.

[hovsater]

@zimbatm I can definitely take a swing at a bigger refactoring. I can also add deprecation warnings or errors, whatever you prefer. Do you follow any specific guidelines at Pusher when it comes to deprecating parts of an API?

Regarding the docs, I can update them as well. What documentation format are you using? I really like TomDoc.

[mdpye]

I agree that it will take much longer to land something that's not backwards compatible (documentation and example code changes and all), and it would be valuable to have basic checking on what exists.

But I also think the next major version should have a clean solution and continue to evolve towards a generally cleaner codebase.

I'm not the library maintainer, this is just my two cents. We should ask @leggetter if he thinks a breaking change is acceptable, as he'd likely be involved in co-ordinating the release of such a thing.

[leggetter]

Deprecating the globals and only supporting passing the credentials into an initializer certainly seems like a good approach. Most other Pusher HTTP libraries take this approach.

I'm not 100% sure about the benefits of requiring a Configuration object for required parameters unless there's a strong likelihood that those required params are going to change. My vote would probably be for named params for required params (or a hash if the languages support named params since it clarifies the credentials that are being passed - people get id/key/secret mixed up quite often) and an optional configuration object for customisations e.g. changing default behaviour.

We would need to look into what assets have to be changed to promote the use of the initializer over the global approach.

• pusher.com
• Pusher dashboard
• blog.pusher.com Tutorials
• External tutorials
• Sample code/apps/demos (search GitHub)
• Presentations

In summary, I'd suggest:

• Deprecate global config
• Potentially add a check that config has been set for each operation (throw exception if not) for now since it's not that much effort but improves usability
• Use named params or a hash in the initializer (already supported)
• Create Configuration object for optional configuration/customisation
• Increment major version
• Build a full list of the assets we're going to have to update to promote the use of the initializer/make it the default and make the changes

[zimbatm]

Just pushed a minimal fix to improve the current situation on master.