search

zabbix-tools / go-zabbix

Go bindings for the Zabbix API
GNU General Public License v2.0
108 stars 70 forks source link

Feature: Allow to build session manually #2

Closed x1unix closed 6 years ago

x1unix commented 6 years ago

Added BuildSession method that allows creating session manually.

This feature is necessary for Zabbix token caching and re-use.

cavaliercoder commented 6 years ago

This is a reasonable change, though a more idiomatic way to approach this might be to make the Session fields public by making them upper case.

x1unix commented 6 years ago

@cavaliercoder, didn't want to break existing codebase. In the future, I offer to rename NewSession to Login and also add session cache feature.

cavaliercoder commented 6 years ago

Thanks for being cautious. Making these fields public should be a nondestructive change so I'm happy for you to submit this change.

If you choose to rename NewSession, maybe consider keeping it around as an alias to the new Login and noting in the documentation that NewSession is deprecated. This will ensure it continues to work for legacy users and can be removed one day. I'm curious which naming convention existing popular libraries use for establishing a session with remote APIs. Maybe the AWS SDK or Stripe, etc.

I'm interested to see the session cache feature!

x1unix commented 6 years ago

@cavaliercoder, thank you for the feedback. I'll refactor my pull request and make those fields public, also I'll add Login method alias. Maybe I'll also add session cache sub-system.