bug/Dynamic-table-comment-on-syntax

[kaarthik108]

resolves #769
docs N/A

Problem

This will fix the comment syntax for dynamic tables

from comment on dynamic_table to comment on dynamic table

Solution

Just splitting the relation_type based on '_' only on dynamic tables

Checklist

• I have read the contributing guide and understand what's expected of me
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

[cla-bot]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @kaarthik108

[kaarthik108]

#769

[mikealfare]

Thanks for the PR @kaarthik108! I just had two nits, but otherwise I'm good with it as long as tests pass.

[dbeatty10]

@mikealfare a curious question:

Would it simplify the code paths if this:

 DynamicTable = "dynamic_table" 

is updated to this?

 DynamicTable = "dynamic table" 

We can imagine a world in which "this one weird trick" irons out everything nicely, and all the if/elses go away ✨

We can also imagine a word in which it creates a bunch of wrinkles elsewhere that we don't want 🙅

Obviously up to you either way you want to go, but at least a possibility to consider.

[mikealfare]

@mikealfare a curious question:

Would it simplify the code paths if this:

 DynamicTable = "dynamic_table" 

is updated to this?

 DynamicTable = "dynamic table" 

We can imagine a world in which "this one weird trick" irons out everything nicely, and all the if/elses go away ✨

We can also imagine a word in which it creates a bunch of wrinkles elsewhere that we don't want 🙅

Obviously up to you either way you want to go, but at least a possibility to consider.

That's a good question, and one we've discussed a bit due to issues like this one. dbt's interface almost everywhere (I say "almost" because I can't confirm it off the top of my head) is snake case. After discussing, we landed on keeping it snake case for consistency. I also wouldn't want the enum to serve multiple purposes (and I don't like that we currently do that in other places). It should solely focus on identifying the type of relation we're working with. The string representation of the enum also happens to also be the text that gets put into the jinja template most times, specifically for tables in views. However, that obviously doesn't work for dynamic tables and materialized views, which have spaces. It also doesn't even work for tables/views all the time. IIRC, you can't drop a view in postgres by running drop view name_of_my_view; it throws a syntax error. You have to run drop table name_of_my_view. So we can't generalize this to drop {{ relation.type }} {{ relation }}. It also can't be generalized to drop table {{ relation }} because this fails for materialized views. This also falls down once you get to more complicated statements, such as the create statement. We want this to be uniform across all statements for the adapters that we maintain. So while it does create a lot of boilerplate, copy/paste code, we feel this is a better, more consistent, more maintainable approach.