-
Notifications
You must be signed in to change notification settings - Fork 524
Conversation
This is indeed a feature that we need. I don't think that this is the most intuitive way to do this. Why not just always accept the timestamp parameter when creating new points? A simple kwargs pop could be done at helper.py#L105. |
if you do so there then the 'cls._datapoints' will not be set correctly, this will fail: |
Sorry for the long delay :( I have been busy with the ResultSet (which I merged just now). I'll get to this soon! |
no worries ;) |
308a913
to
b65e4a4
Compare
@areski influxdb/helper.py:147:80: E501 line too long (81 > 79 characters) are you still around there to complete entirely this PR ? :) |
@gst here a fix for the flake8 issue Can you be more specific about what you mean by completing entirely the PR? |
nothing more, or yes if not already done : if you can rebase your branch on top of master and update the pr ? |
Let's see if @aviau wants this in, no point we work on this if it's not needed ;) |
right ;) |
Can we have a tests? I'll merge after. |
4a77161
to
cec332f
Compare
@@ -141,6 +144,13 @@ def _json_body_(cls): | |||
"fields": {}, | |||
"tags": {}, | |||
} | |||
if 'timestamp' in point.__dict__ \ |
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.
getattr(point, 'timestamp', None)
would be more pythonic I suppose.
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 do :
ts = getattr(point, 'timestamp', 0)
if ts:
json_point['timestamp'] = ts
...
@areski Do you want to finish this PR, or should we take over? Todo:
The reason we should add a time when it is not specified is explained by #264. Inserting several points with no time will result in only the last being taken. I am open to other suggestions. |
@matt-snider Would you like to take over? |
Sorry for the late response. I will take a look at this and open up another PR when I get some time. |
Point can be specified as either a number of nanoseconds, a python datetime object (with or without timezone) or a string in ISO datetime format. If a time is not specified, the Helper sets the time at the time of assembling the point fields so that multiple unique points with the same tags can be committed simultaneously without them failing to add due to all being assigned the same automatic time by the InfluxDB server. This fix is based upon the discussion in influxdata#130 but also includes the outstanding items for it to be merged. I'm happy to receive suggestions for further ways to add test coverage to this change. This also fixes influxdata#264 and fixes influxdata#259.
Point can be specified as either a number of nanoseconds, a python datetime object (with or without timezone) or a string in ISO datetime format. If a time is not specified, the Helper sets the time at the time of assembling the point fields so that multiple unique points with the same tags can be committed simultaneously without them failing to add due to all being assigned the same automatic time by the InfluxDB server. This fix is based upon the discussion in influxdata#130 but also includes the outstanding items for it to be merged. I'm happy to receive suggestions for further ways to add test coverage to this change. This also fixes influxdata#264 and fixes influxdata#259.
Point can be specified as either a number of nanoseconds, a python datetime object (with or without timezone) or a string in ISO datetime format. If a time is not specified, the Helper sets the time at the time of assembling the point fields so that multiple unique points with the same tags can be committed simultaneously without them failing to add due to all being assigned the same automatic time by the InfluxDB server. This fix is based upon the discussion in influxdata#130 but also includes the outstanding items for it to be merged. I'm happy to receive suggestions for further ways to add test coverage to this change. This also fixes influxdata#264 and fixes influxdata#259.
I'll close this as @matt-snider's PR was merged |
No description provided.