Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding time_extracted and bookmark_properties #55

Merged
merged 11 commits into from
Nov 28, 2017

Conversation

nick-mccoy
Copy link
Contributor

Version 0.2.0 includes the 'time_extracted' and 'bookmark_properties' that correspond to these changes

@@ -11,7 +11,7 @@ class CatalogEntry(object):
def __init__(self, tap_stream_id=None, stream=None,
key_properties=None, schema=None, replication_key=None,
is_view=None, database=None, table=None, row_count=None,
stream_alias=None, metadata=None):
stream_alias=None, metadata=None, bookmark_properties=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not need to change the catalog at all. You can revert all the changes on this file.

@@ -37,9 +37,10 @@ class RecordMessage(Message):

'''

def __init__(self, stream, record, version=None):
def __init__(self, stream, record, time_extracted=None, version=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add time_extracted after version in case someone is calling RecordMessage(stream, record, version).

Copy link
Contributor

Choose a reason for hiding this comment

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

We should require time_extracted to be an "aware" datetime.datetime object. Maybe just add a check here to validate that it's either None or a datetime.datetime with a tzinfo property.

@@ -48,6 +49,8 @@ def asdict(self):
'stream': self.stream,
'record': self.record,
}
if self.time_extracted is not None:
result['time_extracted'] = self.time_extracted
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll need to change this to convert the timestamp to ISO format.

Copy link
Contributor

@mdelaurentis mdelaurentis left a comment

Choose a reason for hiding this comment

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

Looks good now

self.stream = stream
self.record = record
self.version = version
self.time_extracted = time_extracted
if time_extracted is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just do if time_extracted:. You don't need the is not None. I know we have the is not None elsewhere in here but we've since learned that omitting the is not None is the preferred style.

self.time_extracted = time_extracted
if time_extracted is not None:
if not u.is_aware_datetime(time_extracted):
raise Exception("'time_extracted' must be an aware "
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd raise ValueError here.

@@ -50,6 +55,8 @@ def asdict(self):
}
if self.version is not None:
result['version'] = self.version
if self.time_extracted is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

is not None

'type': 'SCHEMA',
'stream': self.stream,
'schema': self.schema,
'key_properties': self.key_properties
}
if self.bookmark_properties is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

is not None

@ccapurso ccapurso merged commit ea4b5ba into master Nov 28, 2017
@ccapurso ccapurso deleted the time_extracted_and_bookmark_properties branch November 28, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants