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

Optimize calendar ops #325

Merged
merged 20 commits into from
Mar 4, 2024
Merged

Optimize calendar ops #325

merged 20 commits into from
Mar 4, 2024

Conversation

javiber
Copy link
Collaborator

@javiber javiber commented Dec 7, 2023

No description provided.

@javiber javiber force-pushed the optimize-calendar-ops branch 5 times, most recently from 232ab6b to 220fd0e Compare December 8, 2023 19:37
@javiber javiber marked this pull request as ready for review December 8, 2023 19:38
@javiber javiber requested a review from achoum December 8, 2023 19:38
@github-actions GitHub Actions
Copy link

Coverage report

Main: 91.16% | PR: 91.23% | Diff: 0.08 ✅

@github-actions GitHub Actions
Copy link

Coverage report

Main: 91.18% | PR: 91.26% | Diff: 0.08 ✅

@javiber
Copy link
Collaborator Author

javiber commented Dec 15, 2023 •
edited
Loading

@achoum @ianspektor I just pushed a major refactor to reuse more code. It made each operator so simple that I ended up consolidating everything under a single calendar.cc.

I'm not sure why the tests are failing on 3.8 and 3.11, I haven't been able to reproduce that locally
They fixed themselves

@javiber javiber requested a review from achoum December 15, 2023 21:13
@javiber javiber mentioned this pull request Dec 15, 2023
Closed
@ianspektor
Copy link
Collaborator

@achoum will wait for your re-review before merging this since there was a big refactor after your initial review

@github-actions GitHub Actions
Copy link

Coverage report

Main: 91.19% | PR: 91.27% | Diff: 0.07 ✅

@javiber javiber requested a review from achoum February 8, 2024 19:52
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024

Coverage report

Main: 91.19% | PR: 91.17% | Diff: -0.02 ⚠️

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024

Coverage report

Main: 91.19% | PR: 91.17% | Diff: -0.02 ⚠️

@javiber javiber mentioned this pull request Feb 21, 2024
Merged
@github-actions GitHub Actions
Copy link

Coverage report

Main: 91.12% | PR: 91.10% | Diff: -0.02 ⚠️

@github-actions GitHub Actions
Copy link

Coverage report

Main: 91.10% | PR: 91.08% | Diff: -0.02 ⚠️

@javiber javiber force-pushed the optimize-calendar-ops branch 2 times, most recently from c5b6336 to c69e2f2 Compare February 28, 2024 16:40
@javiber javiber force-pushed the optimize-calendar-ops branch 3 times, most recently from 40dee6c to ab36c41 Compare March 1, 2024 18:01
@javiber
Fixed TZ with DST on calendar operations
50ba9df
@javiber
Optimized calendar year
85a66cd
@javiber
Moved calendar_year to cpp
224559d
@javiber
Moved calendar_month to c++
c121e57
@javiber
Moved calendar_day_of_month to cpp
de8f39c
@javiber
Moved calendar hour, minute and second to cpp
5dfc401
@javiber
Moved day_of_year to cpp
a89eaf1
@javiber
Moved day of week to cpp
b8380a7
@javiber
Moved isoweek to cpp
156bbf3
@javiber
Fix missing calendar_second and error types
b171fe5
@javiber
Fixed rounding error when parsing float tz offsets
c9e68f5
@javiber
Refactored calendar ops
676648e
@javiber
Refactor of the calendar operations
19a09f5 
- made a base class for the calendar operation implementations to reuse more code
- changed the cpp code to avoid raising exceptions and return error messages instead
- simplified the cpp templating code, a single function that accepts a function eas sufficient
@javiber
Added tests for utc timezone
b565c08
@javiber
Added missing changelog
3f9b95f
@javiber
Pass argument tz as reference in parse_tz
8d78e46
@javiber
Return optional string when there is no errors
e029963
@javiber
Added a few more tests for calendar operations
5e96f18
@javiber
Moved calendar utility functions to tick_calendar_utils
e3f49d9 
Also addressed the smaller changes requested
@javiber
Reverted changes to tick_calendar_utils
9dae7ad 
Found some issues that couldn't solve
@javiber
Copy link
Collaborator Author

javiber commented Mar 4, 2024

@achoum I ran into some issues when moving my utility functions to tick_calentar_utils. I merge this PR anyway as it was long overdue, we can discuss about the utility functions and debug when you return

@javiber javiber merged commit e59ae4f into main Mar 4, 2024
21 of 22 checks passed
@javiber javiber deleted the optimize-calendar-ops branch March 4, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achoum achoum achoum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@javiber @ianspektor @achoum