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

adding date() and datetime() short hand macros #112

Merged

Conversation

gregology
Copy link
Contributor

Problem

It takes a lot of Jinja boilerplate to generate simple datetime objects.

{% set foo = modules.datetime.datetime(1997, 9, 29, 6, 14, tzinfo=modules.pytz.timezone('UTC')) %}

Compiles as 1997-09-29 06:14:00+00:00

Solution

The PR introduces datetime() and date() macros to reduce boilerplate and increase code readability.

{% set foo = dbt_date.datetime(1997, 9, 29, 6, 14) %}

Compiles as 1997-09-29 06:14:00+00:00

This halves the character count to produce a datetime object.

The datetime() macro defaults to timezone from the dbt_date:time_zone project variable.

The date() macro doesn't reduce the Jinja boilerplate as much but it is useful for consistency.

{% set foo = dbt_date.date(1997, 9, 29) %}

Compiles as 1997-09-29

@clausherther
Copy link
Contributor

Hi @gregology - sorry for the late reply! This sounds like a really good idea, thanks! Any way we can add this to one of the integration tests so we can make sure these macros are covered?

@gregology gregology force-pushed the adding_date_and_datetime_shorthands branch from d4078b5 to 87b1adf Compare October 23, 2023 16:54
@gregology
Copy link
Contributor Author

Cheers @clausherther! Is this testing pattern suitable?

@clausherther
Copy link
Contributor

clausherther commented Oct 23, 2023

@gregology pushed a couple of changes to get tests to pass. The return value of the macro has to be a string, also there is no millisecond argument on datetime, but there is microsecond 🤷
Also, added a param for tz in case folks want to override for each call.
Let me know what you think!

@gregology
Copy link
Contributor Author

Good call on the tz param and good catch on the missing millisecond argument 👍

I have refactored the macros to output date & datetime objects using casting in the tests. Our use case for this is mainly in testing where Pythonic syntax for datetime manipulation can be more readable.

Is this test coverage and pattern suitable?

Copy link
Contributor

@clausherther clausherther left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@clausherther
Copy link
Contributor

@gregology sorry, one last thing please: could we add these to the bottom of the README, maybe after Fiscal Calendar (and add them to the top index)? Even though they're utility macros, we're expecting folks to use them directly. Particularly, the fact that they don't return strings will trip up people and I'm sure we'll get questions/PRs to convert them. So, a README entry could head that off. Thanks!

@gregology
Copy link
Contributor Author

Good point @clausherther, I have updated the README to document the macros and included reasoning for outputting a date / datetime object. Also, nice use of the anchor links 👍 Thanks for your patience and awesome tool mate!

@clausherther clausherther merged commit 7a7cd80 into calogica:main Oct 24, 2023
4 checks passed
@clausherther
Copy link
Contributor

I'll cut a release later today. Thanks!

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