optimizely / python-sdk

Python SDK for Optimizely Feature Experimentation and Optimizely Full Stack (legacy)
https://docs.developers.optimizely.com/experimentation/v4.0.0-full-stack/docs/python-sdk
Apache License 2.0
32 stars 36 forks source link

feat: add odp rest api manager #398

Closed Mat001 closed 2 years ago

Mat001 commented 2 years ago

Summary

Test plan

Issues

Mat001 commented 2 years ago

@jaeopt @andrewleap-optimizely I hope exception for invalid URL is handled ok. Request.Exception's class InvalidURL is not comprehensive at all. For example it doesn't catch https://api.zaius..com or XXhttps://api.zaius.com. For cases like these two I had to add other exceptions. If better idea how to guard against an invalid url lmk

I'm not super clear how swift does it, it seems it checks for exactly that url string? guard let url = URL(string: "\(apiHost)/v3/events") else { let canRetry = false completionHandler(.odpEventFailed("Invalid url", canRetry)) return }

andrewleap-optimizely commented 2 years ago

I'm not super clear how swift does it, it seems it checks for exactly that url string? guard let url = URL(string: "\(apiHost)/v3/events") else { let canRetry = false completionHandler(.odpEventFailed("Invalid url", canRetry)) return }

We can accomplish something similar using urllib.parse.urlparse

jaeopt commented 2 years ago

@jaeopt @andrewleap-optimizely I hope exception for invalid URL is handled ok. Request.Exception's class InvalidURL is not comprehensive at all. For example it doesn't catch https://api.zaius..com or XXhttps://api.zaius.com. For cases like these two I had to add other exceptions. If better idea how to guard against an invalid url lmk

I'm not super clear how swift does it, it seems it checks for exactly that url string? guard let url = URL(string: "\(apiHost)/v3/events") else { let canRetry = false completionHandler(.odpEventFailed("Invalid url", canRetry)) return }

@Mat001 I understand that RequestException will catch these all. I don't think we need this fine level of error handling, since this is not expected to happen (apiHost we control). Can be cleaned up if needed. Swift checks url only for lang level requirement for optional filtering.

Mat001 commented 2 years ago

@jaeopt @andrewleap-optimizely ok. I refactored. I got rid of the specific URL parsing exceptions and made it such that generic RequestEception catches invalid URL errors (may not include urlparse errors), but it guard against quite a few. And since we control the URL I wouldn't worry about this one, if that's ok