Closed phantomcosmonaut closed 4 years ago
@phantomcosmonaut The change looks good. I suggest to merge the change to the master branch instead - dev branch is where development by Tableau happens. It may take some time before dev is merged to master and a new release is created.
I originally cloned the dev branch thinking I'd push changes to that :/ so if I submit a new pull request on your master branch it will include other modifications on that file I did not make. Do you want me to go ahead with that or clone the master branch and make my changes?
It is fine, we can bring the change to dev branch. The only thing - I can't tell when the fix will make it to a release and package.
Unit tests all passed. I modified your regex to be more compact and made some updates to docstrings. It also looked like the query_timeout attribute was not being set correctly because it was initially set to self but in the property wrapper it was being set to self._service and the latter seems correct. I also made the query_timeout parameter more robust by filtering for argument type before setting. In a previous issue, someone brought up that there was no remove endpoint function so I added one. It works: '[INFO] (web.py:web:2106): 204 DELETE /endpoints/Foo (127.0.0.1) 9.97ms', but it appears to cause a server-side error: '[ERROR] (base_handler.py:base_handler:120): Error submitting update model request: error=AttributeError : 'ContextLoggerWrapper' object has no attribute 'info''. However, client.get_status() is resolved when the server is restarted.