-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
chore: optimizes time variables. #1242
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1242 +/- ##
=======================================
Coverage 81.67% 81.68%
=======================================
Files 168 168
Lines 9757 9762 +5
=======================================
+ Hits 7969 7974 +5
Misses 1537 1537
Partials 251 251
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tx.variables.timeMin.Set(timeOnly[3:5]) | ||
tx.variables.timeSec.Set(timeOnly) | ||
|
||
y, m, d := timestamp.Date() |
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.
timestamp.[Day|Month|Year]()
calls t.date(true)
and ignores pieces of output, hence using timestamp.Date()
uses all values and saves time.
|
||
timeOnly := timestamp.Format(time.TimeOnly) | ||
tx.variables.time.Set(timeOnly) | ||
tx.variables.timeHour.Set(timeOnly[0:2]) |
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.
👅
tx.variables.timeDay.Set(strconv.Itoa(d)) | ||
tx.variables.timeMon.Set(strconv.Itoa(int(m))) | ||
tx.variables.timeYear.Set(strconv.Itoa(y)) |
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.
Do we save some cycles by converting at read time instead? Or the optimization it is negligible?
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.
We cannot convert at read time unless lazy collections.
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.
You can. Instead of doing strconv.Itoa(y)
, you just store y
.
Then, when reading, you return strconv.Itoa(y)
.
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.
timeYear
is a Single variable so you store strings.
Follows #1223
This PR adds a benchmark and also optimization for time variables
Before
After
cc @geoolekom
Make sure that you've checked the boxes below before you submit PR:
Thanks for your contribution ❤️