-
Notifications
You must be signed in to change notification settings - Fork 35
Replace pendulum with whenever #5820
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
Conversation
CodSpeed Performance ReportMerging #5820 will not alter performanceComparing Summary
|
f7b8e63 to
16f3e39
Compare
16f3e39 to
2f3b86f
Compare
5831b5e to
b766d46
Compare
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.
Added some comments but looks good to me.
| async def to_graphql(self, *args: Any, **kwargs: Any) -> DateTime: # noqa: ARG002 | ||
| return self.obj | ||
| async def to_graphql(self, *args: Any, **kwargs: Any) -> datetime: # noqa: ARG002 | ||
| return self.to_datetime() |
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.
Is this method used anywhere?
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.
Yes
| from neo4j.graph import Path as Neo4jPath | ||
| from neo4j.graph import Relationship as Neo4jRelationship | ||
| from pendulum import Interval | ||
| from whenever import TimeDelta |
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.
It would be nice to get rid of this import as well, if we can map it to a TimeDelta from our own TimeStamp class.
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.
agreed but it sounds like a bigger effort
|
|
||
| @property | ||
| def time_range(self) -> Interval: | ||
| def time_range(self) -> TimeDelta: |
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.
As per the updates in the SDK the time_range here and on line 420 will raise a deprecation warning due to us keeping the code in the function intact.
return self.to_time.obj - self.from_time.objWe don't want to use ._obj instead but perhaps we need some other method. Or some dundermethods on our own class so that it's possible to instead run:
return self.to_time - self.from_timeThere 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.
Good catch, I fixed it
b766d46 to
eaefaaa
Compare
eaefaaa to
e149a20
Compare
Related to opsmill/infrahub-sdk-python#256
This PR replaces the library
pendulumwhich is not maintained anymore withwheneverfordatetimemanagement.In the process, I took the opportunity to use our own
Timestampclass in more places instead of using a third party library, this will help the migration to another library in the future (I hope we don't have too)