-
Notifications
You must be signed in to change notification settings - Fork 984
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 Benoit's re commit dbt devblog #1786
Conversation
✅ Deploy Preview for docs-getdbt-com ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Just a quick comment that we should remove "(asciinema link we could put in the post https://asciinema.org/a/lTmefht77ZEr6kmP7DymaxjRF)" or replace the image with the asciinema GIF |
Good catch, Benoit! I removed that comment. |
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.
@b-per this is really top-notch and very educational. I had a blast learning and trying it out.
I used this pull request to try things out -- nearly everything worked flawlessly. Comments note in the few places that didn't work for me.
My main recommendations:
- Remove tāngata since I couldn't get it to work 😢
- Consider if more text can be formatted as
code
orcode_related_names
-- there seemed to be quite a few places that I personally normally format with backticks
I'm going to "request changes", but feel free to take or leave any of my suggestions. I feel good about approving once you give those an initial look.
|
||
It is important though to keep in mind a good balance between setting enough rules and automation to ensure a project of good quality and setting too many of them, taking time from more value-added work and potentially slowing down the overall analytics development process. | ||
|
||
![A diagram that adds additional steps to the original diagram shown in the beginning of the article. This solution includes a cyclical step for continuously adding new rules and leveraging a SQL linter.](/img/blog/2022-07-26-pre-commit-dbt/next-strategy.png) |
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.
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.
I will check it tomorrow. Thanks for the suggestion!
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.
Thanks @dbeatty10
I followed what you suggested and just used the same style for the diagram. I don't know how to update the image though. Could you help?
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.
I can get that image updated for you! I'll comment here when I've committed the change
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.
Okay, the new image should be reflected in the content now!
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.
Looks awesome @b-per ! Thanks @johnblust !
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.
Cool, if we feel like this is good to go, can we get approval @dbeatty10?
Co-authored-by: Doug Beatty <[email protected]>
Co-authored-by: Doug Beatty <[email protected]>
Co-authored-by: Doug Beatty <[email protected]>
Co-authored-by: Doug Beatty <[email protected]>
Co-authored-by: Doug Beatty <[email protected]>
Co-authored-by: Doug Beatty <[email protected]>
…into add-pre-commit-dbt-devblog
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.
Stellar work @b-per + @johnblust 🥇
@b-per If this looks good to you, let me know and I'll update the publication date and get this merged! Social promotion is still set for this Friday! |
All good from my side as well! |
I meant to ask this earlier: Is there a reason why we aren't embedding the asciinema GIF in the post? asciinema does two cool things:
Obviously not a show-stopper to leave it out, just something that is kinda cool if we can get it in. |
No reason except that embedding requires a |
I see! Thank you for explaining @b-per 🙌 The image + external link is a good solution given that context. Confirmed with experimentationI experimented with embedding via the Quick google search didn't yield any quick-n-easy solutions. Just areas to dig into further like: |
Checklist
If you added new pages (delete if not applicable):
website/sidebars.js