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 Benoit's re commit dbt devblog #1786

Merged
merged 19 commits into from
Aug 3, 2022
Merged

Conversation

johnblust
Copy link
Contributor

@johnblust johnblust commented Jul 22, 2022

Checklist

If you added new pages (delete if not applicable):

  • The page has been added to website/sidebars.js
  • The new page has a unique filename

@johnblust johnblust requested review from jasnonaz and b-per July 22, 2022 16:17
@johnblust johnblust requested review from a team, annafil and runleonarun as code owners July 22, 2022 16:17
@netlify
Copy link

netlify bot commented Jul 22, 2022

Deploy Preview for docs-getdbt-com ready!

Name Link
🔨 Latest commit 191d4d7
🔍 Latest deploy log https://app.netlify.com/sites/docs-getdbt-com/deploys/62ea7caff1754b0009de6422
😎 Deploy Preview https://deploy-preview-1786--docs-getdbt-com.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added developer blog This content fits on the developer blog. size: large This change will more than a week to address and might require more than one person labels Jul 22, 2022
@b-per
Copy link
Contributor

b-per commented Jul 25, 2022

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

@johnblust
Copy link
Contributor Author

Good catch, Benoit! I removed that comment.

@dbeatty10 dbeatty10 self-requested a review July 26, 2022 19:26
Copy link
Contributor

@dbeatty10 dbeatty10 left a 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 or code_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.

website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

You are a master at making compelling images, and WAY better at this than me.

Maybe something like this to highlight which steps are already covered and where they should focus next?
image

Copy link
Contributor

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!

Copy link
Contributor

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?

image

Copy link
Contributor Author

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

Copy link
Contributor Author

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!

Copy link
Contributor

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 !

Copy link
Contributor Author

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?

website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
website/blog/2022-07-26-pre-commit-dbt.md Outdated Show resolved Hide resolved
@b-per b-per requested a review from KiraFuruichi as a code owner August 1, 2022 16:26
@b-per b-per requested a review from dbeatty10 August 2, 2022 07:29
Copy link
Contributor

@dbeatty10 dbeatty10 left a 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 🥇

@johnblust
Copy link
Contributor Author

@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!

@b-per
Copy link
Contributor

b-per commented Aug 3, 2022

All good from my side as well!

@johnblust johnblust merged commit e16ae4b into current Aug 3, 2022
@johnblust johnblust deleted the add-pre-commit-dbt-devblog branch August 3, 2022 13:54
@dbeatty10
Copy link
Contributor

@b-per:

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

@johnblust

Good catch, Benoit! I removed that comment.

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:

  1. Shows what the commands and output looks like in action
  2. Allows the commands to be copied-n-pasted

Obviously not a show-stopper to leave it out, just something that is kinda cool if we can get it in.

@b-per
Copy link
Contributor

b-per commented Aug 3, 2022

No reason except that embedding requires a <script></script> tag (see here) and I don't think that we can add it in Markdown. If it is technically possible to somehow embed it rather than have it as an image + external link this would definitely be better!

@dbeatty10
Copy link
Contributor

I see! Thank you for explaining @b-per 🙌

The image + external link is a good solution given that context.

Confirmed with experimentation

I experimented with embedding via the <script> tag per asciinema's documentation and confirmed it didn't work.

Quick google search didn't yield any quick-n-easy solutions. Just areas to dig into further like:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer blog This content fits on the developer blog. size: large This change will more than a week to address and might require more than one person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants