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

Switch source of IDs/adjectives for name generator to GitHub #26

Merged
merged 2 commits into from
May 16, 2024

Conversation

dfsnow
Copy link
Member

@dfsnow dfsnow commented May 16, 2024

This PR is a quick follow-up to ccao-data/data-architecture#451. It updates the ccao package to point to the new GitHub tables as the source of names/adjectives for the ccao_generate_id() function.

@dfsnow dfsnow requested a review from jeancochrane May 16, 2024 03:54
@dfsnow dfsnow requested a review from a team as a code owner May 16, 2024 03:54
@@ -2,7 +2,7 @@
# R specific hooks: https://github.com/lorenzwalthert/precommit
repos:
- repo: https://github.com/lorenzwalthert/precommit
rev: v0.3.2.9013
rev: v0.4.2
Copy link
Member Author

Choose a reason for hiding this comment

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

Totally unrelated to this PR but for the fact that the check failed due to a super old version.

Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

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

Looks good! Moving forward, am I right to assume that we'll reuse the implementation in ccao in other projects? Or is each project going to reimplement this same read_csv logic?

Comment on lines +3 to +4
"https://github.com/raw/ccao-data/data-architecture/master/",
"dbt/seeds/ccao/ccao.adjective.csv"
Copy link
Contributor

Choose a reason for hiding this comment

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

[Thought, non-blocking] I think this is probably the best option, but it makes me a little bit nervous that the behavior of the name generator could change for a single installed version of the package. Not sure what to do about that, but I wanted to voice my uncertainty!

@dfsnow
Copy link
Member Author

dfsnow commented May 16, 2024

Looks good! Moving forward, am I right to assume that we'll reuse the implementation in ccao in other projects? Or is each project going to reimplement this same read_csv logic?

Well, the res and condo models will use the ccao package implementation, while the land and sales val models will need to use some sort of read_csv() function. IMO this is fine. As long as there's a single source-of-truth for the names, it's okay for the synchronization to be a little janky, this isn't a high stakes thing after all. But curious if you disagree @jeancochrane

@dfsnow dfsnow merged commit 0cb8255 into master May 16, 2024
10 checks passed
@dfsnow dfsnow deleted the dfsnow/switch-id-source branch May 16, 2024 16:19
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.

2 participants