dbt-labs / dbt-codegen

Macros that generate dbt code
https://hub.getdbt.com/dbt-labs/codegen/latest/
Apache License 2.0
459 stars 99 forks source link

Make `generate_source` macro return case-sensitive names #136

Closed jgillies closed 4 months ago

jgillies commented 1 year ago

resolves #112

This is a:

All pull requests from community contributors should target the main branch (default).

Description & motivation

For organizations with strict standards around the letter case of their tables and columns. The existing behavior of the generate_source macro is that it returns all table and column names in lowercase. This makes it less useful for the previously mentioned organizations.

This adds the parameters case_sensitive_tables and case_sensitive_cols to the generate_source macro. Both default to false to preserve existing behavior.

Checklist

gwenwindflower commented 6 months ago

Hey @jgillies -- thanks for your patience with this, we were a bit short staffed while I was on medical leave for several months. This looks awesome and definitely should be added, thank you! πŸ™πŸ»

Couple things for consideration:

Lastly, as you opened this quite a while ago, if your capacity has changed and you don't have the time to get into the above, no worries at all, just let me know and I can see about pushing some more commits to this branch. Would prefer to help you get this across the finish line yourself though if you're in for it!

Here to help with anything or any questions, just @ me.

gwenwindflower commented 5 months ago

Also, I went ahead and resolved the conflicts with main that have cropped up in the months this was waiting, so one less hurdle to worry about.

jgillies commented 5 months ago

Hey @jgillies -- thanks for your patience with this, we were a bit short staffed while I was on medical leave for several months. This looks awesome and definitely should be added, thank you! πŸ™πŸ»

Couple things for consideration:

  • Did you run the test suite? It looks like no tests were edited in this PR and a couple are failing now, would be worth running make test and checking those out, and perhaps looking at the other tests that start with test_generate_source. Don't hesitate to let me know if you get stuck with setting it up and running the tests, it can be a little confusing.
  • Would you be up for expanding this to include Databases and Schemas? I think they should also probably be made optionally case sensitive.

Lastly, as you opened this quite a while ago, if your capacity has changed and you don't have the time to get into the above, no worries at all, just let me know and I can see about pushing some more commits to this branch. Would prefer to help you get this across the finish line yourself though if you're in for it!

Here to help with anything or any questions, just @ me.

Hi @gwenwindflower! I'm not able to jump back into this at the moment, so if someone else can push it across the finish line I'm totally OK with that. Otherwise, I can spend some time on it within the next couple months.

gwenwindflower commented 5 months ago

All good @jgillies -- thanks for letting us know, we've got it! Appreciate the work, patience, and thought to get it to here! ❀️

pnadolny13 commented 5 months ago

@jgillies @gwenwindflower glad to see this PR! I've just run into the same issue.

Is there anything I can help with to get this over the finish line? I'm happy to contribute! πŸ˜„

gwenwindflower commented 5 months ago

hey @pnadolny13 πŸ‘‹πŸ» ! that would be super welcome! take a look here for how package development works and if that all makes sense, then the things remaining are:

very happy to answer any questions!

pnadolny13 commented 4 months ago

@gwenwindflower I created a new PR https://github.com/dbt-labs/dbt-codegen/pull/168 that should have all the remaining items, let me know if you have any feedback!

Thanks @jgillies for creating this PR! If its ok with you then I think we can close this and get https://github.com/dbt-labs/dbt-codegen/pull/168 across the finish line πŸ˜„ .

gwenwindflower commented 4 months ago

Closed because of #168