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.

Sign up for GitHub

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

Jump to bottom

Replace pendulum with whenever #256

Merged
merged 1 commit into from
Feb 26, 2025
Merged

Replace pendulum with whenever #256

merged 1 commit into from
Feb 26, 2025

Conversation

dgarros
Copy link
Contributor

@dgarros dgarros commented Jan 30, 2025 •
edited
Loading

fixes #255

This PR replaces pendulum with whenever to manage datetime, mainly as part of the Timestamp class.

This change allows us to re-enable the support for Python 3.13, since we haven't shipped a release without 3.13 support, I've updated the changelog for #251

In the process the Timestamp object has been slightly refactored

  • Direct access to obj has been deprecated, instead it's recommended to use to_datetime()
  • add_delta() has been deprecated, in favor of add() and substract() that are more inline with whenever

@codecov Codecov
Copy link

codecov bot commented Jan 30, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.11111% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/timestamp.py 90.16% 6 Missing ⚠️
infrahub_sdk/utils.py 90.90% 2 Missing ⚠️ 
@@                 Coverage Diff                 @@
##             infrahub-develop     #256   +/-   ##
===================================================
  Coverage                    ?   70.53%           
===================================================
  Files                       ?       81           
  Lines                       ?     7561           
  Branches                    ?     1469           
===================================================
  Hits                        ?     5333           
  Misses                      ?     1849           
  Partials                    ?      379
Flag Coverage Δ
integration-tests 22.27% <0.00%> (?)
python-3.10 45.62% <68.88%> (?)
python-3.11 45.60% <66.66%> (?)
python-3.12 45.62% <68.88%> (?)
python-3.13 45.60% <66.66%> (?)
python-3.9 44.46% <68.88%> (?)
python-filler-3.12 24.07% <22.22%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/ctl/branch.py 77.14% <100.00%> (ø)
infrahub_sdk/ctl/utils.py 66.66% <ø> (ø)
infrahub_sdk/exceptions.py 81.11% <100.00%> (ø)
infrahub_sdk/utils.py 79.91% <90.90%> (ø)
infrahub_sdk/timestamp.py 81.05% <90.16%> (ø)

@dgarros dgarros force-pushed the dga-20250130-whenever branch from 41aa45d to cceabf0 Compare February 23, 2025 10:18
@dgarros dgarros changed the base branch from develop to infrahub-develop February 23, 2025 10:18
@dgarros dgarros force-pushed the dga-20250130-whenever branch from cceabf0 to ce24e52 Compare February 23, 2025 10:19
@github-actions github-actions bot added the group/ci Issue related to the CI pipeline label Feb 23, 2025
@dgarros dgarros mentioned this pull request Feb 23, 2025
Merged
@dgarros dgarros force-pushed the dga-20250130-whenever branch from ce24e52 to 702a5f1 Compare February 24, 2025 09:09
@github-actions github-actions bot removed the group/ci Issue related to the CI pipeline label Feb 24, 2025
@dgarros dgarros force-pushed the dga-20250130-whenever branch 2 times, most recently from 8234ed7 to 97986b8 Compare February 26, 2025 06:54
@github-actions github-actions bot added the group/ci Issue related to the CI pipeline label Feb 26, 2025
@dgarros dgarros requested a review from a team February 26, 2025 07:00
@dgarros dgarros marked this pull request as ready for review February 26, 2025 07:03
ogenstad
ogenstad approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

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

We should also have a news fragment stating that the installation on Python 3.13 was fixed.

changelog/255.fixed.md Outdated
@@ -0,0 +1 @@
Refactor Timestamp to use `whenever` instead of `pendulum` and extend Timestamp with add(), subtract(), and to_datetime(). Direct access to `obj` and `add_delta` have been deprecated and will be removed in a future version.
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 we could split this up into two fragments where we also use the special one for things marked for deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I forgot about the deprecated section

infrahub_sdk/timestamp.py
UserWarning,
stacklevel=2,
)
return self._obj
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 suggest creating a follow up issue for when this should be removed and tie it into a future milestone so we know when to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

infrahub_sdk/timestamp.py

return DateTime.now(tz="UTC").subtract(**params)
raise TimestampFormatError(f"Invalid time format for {value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it was like this before but I think the error definition should live in exceptions.py like the other core SDK exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I moved it

ajtmccarty
ajtmccarty approved these changes Feb 26, 2025
View reviewed changes
infrahub_sdk/utils.py Outdated
except TimestampFormatError:
return None

delta: TimeDelta = Timestamp()._obj.difference(time_value._obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these use to_datetime()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not datetime, the goal is to access the ZonedDateTime here
I added a new get_obj() instead of accessing it directly

@dgarros dgarros force-pushed the dga-20250130-whenever branch from 97986b8 to d778262 Compare February 26, 2025 20:10
@dgarros dgarros force-pushed the dga-20250130-whenever branch from d778262 to a2a496f Compare February 26, 2025 20:16
@dgarros dgarros merged commit 55be7bb into infrahub-develop Feb 26, 2025
14 checks passed
@dgarros dgarros deleted the dga-20250130-whenever branch February 26, 2025 20:24
@fatih-acar fatih-acar mentioned this pull request Feb 27, 2025
Merged
@dgarros dgarros mentioned this pull request Mar 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogenstad ogenstad ogenstad approved these changes

@ajtmccarty ajtmccarty ajtmccarty approved these changes

Assignees
No one assigned
Labels
group/ci Issue related to the CI pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dgarros @ogenstad @ajtmccarty