Skip to content
This repository was archived by the owner on Oct 29, 2024. It is now read-only.

Data arg in write() can be single string #492

Merged
merged 5 commits into from
Aug 22, 2017
Merged

Data arg in write() can be single string #492

merged 5 commits into from
Aug 22, 2017

Conversation

baftek
Copy link
Contributor

@baftek baftek commented Aug 19, 2017

data argument in write() was expected to be list of strings, now it can be single string as well

baftek added 3 commits August 19, 2017 19:15
data argument in write() was expected to be list of strings, now it can be single string as well
Copy link
Contributor

@sebito91 sebito91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, thank you for the catch!

@@ -284,15 +285,18 @@ def write(self, data, params=None, expected_response_code=204,
headers = self._headers
headers['Content-type'] = 'application/octet-stream'

if params:
if params and 'precision' in params:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check here, this will default to None.

In [1]: params = {'one': 'two'}

In [2]: if params:
      :     precision = params.get('precision')
      : else:
      :     precision = None
      :

In [3]: precision

In [4]: params
Out[4]: {'one': 'two'}

In [5]: type(precision)
Out[5]: NoneType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I thought I got exception from this but apparently missing key with get() gives Null.
You're right.

precision = params.get('precision')
else:
precision = None

if protocol == 'json':
data = make_lines(data, precision).encode('utf-8')
elif protocol == 'line':
data = ('\n'.join(data) + '\n').encode('utf-8')
if type(data) == list:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this slightly as follows:

if not isinstance(data, list):
    data = [data]

data = ('\n'.join(data) + '\n').encode('utf-8')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super-experienced Pythonist, however I think that this is not the best solution. Or is it?
Since sequence is requested, user may provide both list or tuple. In this case if tuple is found it will be not casted to list but put into list. Then the following join will fail because instead of str it will find tuple. Cast by list() would be much better, because str will be put into list, and tuple converted into list. Win-win I guess. If any other data structure was provided - exception is justified.

@@ -292,6 +293,8 @@ def write(self, data, params=None, expected_response_code=204,
if protocol == 'json':
data = make_lines(data, precision).encode('utf-8')
elif protocol == 'line':
if isinstance(data, str):
data = list(data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not do what you expect, instead it'll split out your string into a list as follows:

In [4]: tester = "onetwothree"

In [5]: [tester]
Out[5]: ['onetwothree']

In [6]: list(tester)
Out[6]: ['o', 'n', 'e', 't', 'w', 'o', 't', 'h', 'r', 'e', 'e']

Please replace with data = [data] and that should suffice.

@sebito91
Copy link
Contributor

This is great, thank you @baftek!

Copy link
Contributor

@sebito91 sebito91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your attention to detail @baftek!

sebito91 added a commit to sebito91/influxdb-python that referenced this pull request Aug 22, 2017
@sebito91 sebito91 merged commit 265d147 into influxdata:master Aug 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants