rc10house commented on code in PR #47:
URL: https://github.com/apache/flagon-distill/pull/47#discussion_r1671032094
##########
tests/test_log.py:
##########
@@ -44,6 +44,10 @@ def test_log_constructor():
pageUrl = test_log.data.page_url
assert pageUrl == "https://github.com/apache/flagon/tree/master/docker";
+ id = test_log.id
Review Comment:
Might want to make this it's own test just for readability if there were to
be errors ever, but it is not that big of a deal since the timestamp and log
parsing are tightly coupled.
##########
distill/core/log.py:
##########
@@ -46,7 +47,8 @@ def __init__(self, data: Union[str, JsonDict],
schema=UserAleSchema):
raise TypeError("ERROR: " + str(type(data)) + " data should be
either a string or a JsonDict")
self.data = schema(**data)
- # TODO: need to create ID field here on object initialization
+ self.id = PKSUID("log", schema._timestamp(self.data))
Review Comment:
Bake the userId into the PKSUID as Evan mentioned in the call to make the
log truly globally unique. Ex: log_userId_hash
##########
tests/test_log.py:
##########
@@ -44,6 +44,10 @@ def test_log_constructor():
pageUrl = test_log.data.page_url
assert pageUrl == "https://github.com/apache/flagon/tree/master/docker";
+ id = test_log.id
+ assert id.get_timestamp() == 1719530111079 // 1000
Review Comment:
As long as this passes it is fine, but I do not use floor division when
parsing the timestamp. Just a nit pick though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@flagon.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@flagon.apache.org
For additional commands, e-mail: notifications-h...@flagon.apache.org
