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

Add utils for logging to the command line #131

Merged
merged 2 commits into from
Jul 11, 2019
Merged

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Apr 4, 2019

I thought about how to test this, but it gets pretty hard given the output depends on the clock time. I wanted to do a regex match on the output, but Jinja doesn't support this.

In the end, I thought it would just be easiest to test if the output is a string 🙂

@clrcrl clrcrl force-pushed the feature/logger-utils branch 2 times, most recently from 6cb7c10 to 5aeb613 Compare May 31, 2019 22:24
@clrcrl clrcrl marked this pull request as ready for review May 31, 2019 22:25
@clrcrl clrcrl requested a review from drewbanin May 31, 2019 22:25
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Cool PR @clrcrl! How would you feel about adding a log_info macro that just goes ahead and invokes:

{{ log(dbt_utils.pretty_output_msg(<the message>), info=True) }}

It seems like this is the intended use-case anyway, right?

README.md Outdated

```
11:07:28 | 1 of 1 START table model analytics.fct_orders........................ [RUN]
+ 11:07:31 | my pretty message
Copy link
Contributor

Choose a reason for hiding this comment

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

When I saw this PR, I imagined the output would instead look like:

11:07:31 + my pretty message

What's the motivation behind placing the timestamp on the right side of the divider?

Copy link
Contributor Author

@clrcrl clrcrl Jun 2, 2019

Choose a reason for hiding this comment

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

Ah, so I was largely following the pattern from here (and in fact means to swap that code out).

I like having it indented as it makes it clear that it's being called from within a model (to me at least). But happy to change it if you feel it's unintuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I'd go ahead and swap it around I think unless you feel strongly that it should work the way you've already implemented

@clrcrl
Copy link
Contributor Author

clrcrl commented Jun 2, 2019

Yah can for sure add a log_info macro – I back-and-forth-ed as to whether I should actually call the log function or not, so very happy to do that :)

Is the naming okay? I took inspiration from pretty print, but am also happy to rename it.

@drewbanin
Copy link
Contributor

I'd maybe just consider changing pretty_output_msg to pretty_log_format or similar, but otherwise I'm happy with these pretty names :)

@clrcrl clrcrl requested a review from drewbanin June 7, 2019 19:31
@clrcrl clrcrl force-pushed the feature/logger-utils branch from d0b35ea to 01d77f7 Compare June 10, 2019 19:08
@clrcrl clrcrl force-pushed the feature/logger-utils branch from 01d77f7 to 1c109a4 Compare June 21, 2019 16:17
@clrcrl clrcrl force-pushed the feature/logger-utils branch from 1c109a4 to d559a3e Compare June 21, 2019 16:17
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

LGTM

@clrcrl clrcrl merged commit 1cbcc67 into master Jul 11, 2019
@clrcrl clrcrl deleted the feature/logger-utils branch July 11, 2019 19:05
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