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

Make generate_source macro return case-sensitive names #136

Closed

Conversation

jgillies
Copy link

@jgillies jgillies commented Aug 1, 2023

resolves #112

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

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

  • This code is associated with an issue which has been triaged and accepted for development.
  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@@ -44,7 +44,7 @@
{% endif %}

{% for table in tables %}
{% do sources_yaml.append(' - name: ' ~ table | lower ) %}
{% do sources_yaml.append(' - name: ' ~ (table if case_sensitive_tables else table | lower)) %}
Copy link

Choose a reason for hiding this comment

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

Would it make sense to have the name still as lower case but add the uncased as identifier?

Copy link
Author

@jgillies jgillies Nov 2, 2023

Choose a reason for hiding this comment

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

Ah, I never knew about the distinction between table and identifier in sources. Makes sense to use identifier!

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I believe this is assigning the relation's identifier to the table variable, so no change would be necessary to pick up the logic you're suggesting. Does that make sense?

{% set table_list= tables | map(attribute='identifier') %}

@gwenwindflower gwenwindflower self-assigned this Feb 28, 2024
@gwenwindflower
Copy link
Contributor

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.

@gwenwindflower
Copy link
Contributor

gwenwindflower commented Feb 29, 2024

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
Copy link
Author

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

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

@pnadolny13
Copy link
Contributor

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

gwenwindflower commented Mar 20, 2024

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:

  • adding column sensitivity to other properties as listed above
  • adding tests

very happy to answer any questions!

@pnadolny13 pnadolny13 mentioned this pull request Apr 1, 2024
9 tasks
@pnadolny13
Copy link
Contributor

pnadolny13 commented Apr 1, 2024

@gwenwindflower I created a new PR #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 #168 across the finish line 😄 .

@gwenwindflower
Copy link
Contributor

Closed because of #168

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.

Names are automatically casted to lower-case, regardless of capitalisation in database
5 participants