Add Zoho Domain - Githubissues

meyskens commented 5 years ago

First of all thank you very much for creating this project, it really helped me!

This PR allows to set the Zoho TLD which I needed to run against Zoho.eu which is needed in the EU. Feedback welcome!

schmorrison commented 5 years ago

Sorry I took a minute to see this. I’ll review it today and let you know if I need any changes.

Thanks for the PR @meyskens

schmorrison commented 5 years ago

Ok I've got a couple of changes (or justifications if you want to keep things the way they are):

I wonder if the ZohoDomain and baseURL variables can be global variables (rather than constants) and private, then changed via methods within the top level zoho package
```
var (
zohoTLD = "com"
baseURL = "https://accounts.zoho.com/oauth/v2/"
)
.......
```

func SetZohoTLD(s string) { zohoTLD = s oauthBaseURL = fmt.Sprintf("https://accounts.zoho.%s/oauth/v2/", s) }

- I'd prefer to see the ZohoDomain field in the API struct to be named zohoTLD (as shown above), or it can potentially be placed inside the zoho.oauth struct if it makes more sense (I am ambivalent).
- Where you initialize the z.oauth.baseURL I'd prefer it to be within the defined struct as so:

// This will require creating a separate struct called Oauth in the zoho package which will contain the // fields of the nested struct.... you should evaluate this proposal as it may make future variable // assignment more verbose or require additional changes in the codebase. z := Zoho{ client: &http.Client{ Timeout: time.Second 10, Transport: &http.Transport{ Dial: (&net.Dialer{ Timeout: 5 time.Second, }).Dial, TLSHandshakeTimeout: 5 * time.Second, }, }, tokensFile: "./.tokens.zoho", ZohoDomain: "com", // as mentioned this should be changed to zohoTLD oauth: Oauth { baseURL: "https://accounts.zoho.com/oauth/v2/", }, } .......... // And the Oauth struct as so type Oauth struct { scopes []ScopeString clientID string clientSecret string redirectURI string token AccessTokenResponse baseURL string }



That should handle all my preferences, I've given you a few options to decide between so you can pick the way thats most comfortable for you. If you have any questions or reasoning you'd like to discuss you can send a message.

Again, thanks for the PR @meyskens I haven't had time to work on this project lately so every PR helps make this project more feature complete.

Best regards,

schmorrison

meyskens commented 5 years ago

Thanks for the feedback! Will change them as requested, I wasn't fully sure about some of those things either :wink:

meyskens commented 5 years ago

@schmorrison addressed the feedback in a way that I think will cause the least issues with the "subpackages"

schmorrison commented 5 years ago

Ok great.

Just one more proposal, which might not be worth it, would it be more appropriate to move ZohoTLD and the SetZohoTLD method into the API struct from zoho/crm/crm.go? This would allow us to make the ZohoTLD field private as zohoTLD.

Because the Zoho struct is embedded into the API struct this would prevent accessing the field inappropriately, but would require adding this field and method to the API struct of every subpackage.

Again, not sure its worth it, but I will create a new issue where we can discuss it.

Thanks again @meyskens,

schmorrison

schmorrison / Zoho

Add Zoho Domain #14