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.

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

Include get-date helper function #262

Merged
merged 8 commits into from
Oct 12, 2022
Merged

Conversation

zalegrala
Copy link
Contributor

No description provided.

Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you! Would you mind adding some unit tests for this new helper? You can find examples here: https://github.com/mickael-menu/zk/blob/c21c4fc21f7fe75f05d3fc3d09e74ffed41ba2ef/internal/adapter/handlebars/handlebars_test.go#L229

internal/adapter/handlebars/helpers/getdate.go Outdated Show resolved Hide resolved
@zalegrala
Copy link
Contributor Author

Nice, thanks for the review. I'll find some time this week to add tests and address your feedback.

zalegrala and others added 4 commits October 5, 2022 20:03
Co-authored-by: Mickaël Menu <mickael.menu@gmail.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
@zalegrala
Copy link
Contributor Author

I've added a couple tests. Let me know if you'd like me to drop the date calls in the get-date tests. I added them because I mentioned them in the function docs, so thought it was worth leaving in.

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

I've added a couple tests. Let me know if you'd like me to drop the date calls in the get-date tests.
No that's great, thank you.

I think the tests are failing because the helper is using time.Now() in the implementation, so the now context var set in the tests is ignored.

To work around this, and after fixing my other comment to use TimeFromNatural(), you can use an absolute date as input (e.g. {{get-date "2006-01-02T15:04:05"}} instead of a relative one. We just need to test that TimeFromNatural() is called, and can assume that relative dates will work as well.

// {{date (get-date "last week") "timestamp"}}
func RegisterGetDate(logger util.Logger) {
raymond.RegisterHelper("get-date", func(natural string) time.Time {
date, err := naturaldate.Parse(natural, time.Now())
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this in my first review, but you can use the helper TimeFromNatural() (which is part of this project) instead of using directly naturaldate.Parse(). This way it will be able to parse more formats.

Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
@zalegrala
Copy link
Contributor Author

Okay, I've updated. As a side note, I had to add exclude github.com/mattn/go-sqlite3 v2.0.3+incompatible to the go.mod in order to test locally. Looks like some chatter here about it. mattn/go-sqlite3#975

Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
@mickael-menu
Copy link
Member

@zalegrala Thank you for contributing this feature!

I might change the names before the next release, I'll sleep on it:

  • {{date}} -> {{format-date}}
  • {{get-date}} -> {{date}}

@mickael-menu mickael-menu merged commit 404ef9d into zk-org:main Oct 12, 2022
@zalegrala zalegrala deleted the getDateHelper branch October 12, 2022 18:18
@mickael-menu
Copy link
Member

I released a new version and renamed the date helpers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants