-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added comparison dunder methods and some tests #53
Conversation
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.
Only a few small changes from me
distill/core/log.py
Outdated
if isinstance(other, Log): | ||
return self.id < other.id | ||
if isinstance(other, datetime): | ||
return self.id.get_datetime() < other.now() |
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.
other.now()
will get the current datetime, I think instead we should just be able to compare self.id.get_datetime() < other
if other
is a datetime object.
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.
This assumes get_datetime()
returns an actual datetime object, but I don't think it does. Tracking down the method starting from the PKSUID repo to the datetime library docs suggests it just returns a float.
Indeed, testing it in the terminal shows it just returns POSIX time float. I believe datetime objects have a timestamp()
method that will return what we want. But we need to be careful to ensure get_datetime() and other.timestamp()
are operating on the same scale.
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.
@EandrewJones looks to me like it does return a datetime object? "getDatetime() returns a python date object which represents the approximate time (missing microseconds) that the pksuid was created
"
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 found that Ryan's comparison works, while trying to compare get_datetime() to timestamp() gives an error due to the float return value of timestamp.
distill/core/log.py
Outdated
if isinstance(other, Log): | ||
return self.id <= other.id | ||
if isinstance(other, datetime): | ||
return self.id.get_datetime() <= other.now() |
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.
Same as above for every method
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.
Response is the same as my comment above.
Also need to resolve conflicts. |
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.
Looks good to me - @EandrewJones or @Jyyjy can do final look over and merge
[Evan] Closes #43