-
Notifications
You must be signed in to change notification settings - Fork 521
Allow connecting to influxdb running on a path on the server #556
Conversation
Make it possible to connect to the databases on a path on servers. https://someserver.com/myinfluxdb instead of the root of the server.
As far as I can tell the failures in the tests are unrelated in this change and also visible in other merge requests. |
Is anyone looking at these requests? And is there some way to re-run travis? |
sorry about the delay -- looking into CI failures. |
Thanks :) |
influxdb/client.py
Outdated
): | ||
"""Construct a new InfluxDBClient object.""" | ||
self.__host = host | ||
self.__port = int(port) | ||
self.__path = path if not path or path[0] == '/' else '/' + path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're slicing the string here, perhaps it would make sense to first cast the input to a string (similar to the above port cast)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you'e prefer something like:
_path = str(path)
self.__path = _path if not _path or _path[0] == '/' else '/' + _path
Any other suggestion how to make this look better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It dosen't have to be a oneliner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, my suggestion was no longer a one-liner either. What should it be like?
Any chance to get this reviewed soon? I stumbled across this issue in HomeAssistant which uses the library and I'd like to create a patch for it - but that needs this feature released ;) Kind regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it should also handle (and test) if path is None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd create another PR with path=None test.
path=None is tested in the test already, it's the default case. The newly added tests test the other cases. |
I beg to differ. The default is an empty string which actually works. Just added a test with None path:
The test fails:
I have created a PR to your PR that adds the test and the fix (I split the expression since 3 conditions don't fit into one line anymore). |
Test and fix for None path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
would be interested in this PR too |
Thanks for your contribution! |
Thanks for taking it in :) less ugly workarounds on my side. |
Make it possible to connect to the databases on a path on servers.
https://someserver.com/myinfluxdb instead of the root of the server.