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.

Sign up for GitHub

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

Jump to bottom

Renaming Attributes In Metric Spec #5775

Merged
merged 9 commits into from
Sep 9, 2022
Merged

Renaming Attributes In Metric Spec #5775

merged 9 commits into from
Sep 9, 2022

Conversation

callum-mcdata
Copy link
Contributor

@callum-mcdata callum-mcdata commented Sep 6, 2022 •
edited
Loading

resolves #5774

Description

  • Renaming sql to expression
  • Renaming type to calculation_method
  • Renaming expression type metrics to derived

Checklist

@cla-bot cla-bot bot added the cla:yes label Sep 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@callum-mcdata callum-mcdata changed the title making updates - see what fails Renaming Attributes In Metric Spec Sep 6, 2022
@callum-mcdata
Copy link
Contributor Author

Interestingly I am getting this error Could not render {{metric('collective_tenure')}} / {{metric('number_of_people')}} : 'metric' is undefined which seems to suggest that changing the name of the sql field to expression broke the ability to reference metric in a metric sql field. I've tried searching through the codebase but can't seem to figure out why

@callum-mcdata
Copy link
Contributor Author

After some helpful pointers from Jerco, it looks like schema_renderer.py is where my issue was. It was specifically looking for the sql field in the metrics spec in order to render the jinja in that field. Updated and seems to have fixed that issue!

@jtcohen6 jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Sep 7, 2022
@callum-mcdata callum-mcdata marked this pull request as ready for review September 7, 2022 16:43
@callum-mcdata callum-mcdata requested review from a team as code owners September 7, 2022 16:43
@callum-mcdata
Copy link
Contributor Author

Holding off until 9/8 to give the community time to respond.

@callum-mcdata
Copy link
Contributor Author

This PR is ready for review/merge now! The community had no 🔴 flag alarms on the naming so we move forward

@callum-mcdata callum-mcdata mentioned this pull request Sep 8, 2022
Merged
2 tasks
gshank
gshank approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@gshank gshank 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! I do think that these name changes are an improvement.

@callum-mcdata
Copy link
Contributor Author

Perfect, thank you for the review @gshank ! I don't have merge permissions though so would you mind merging it if we've got the reviews we need? Otherwise I can wait for Nathaniel or Colin if we need to get their go ahead as well

@gshank gshank merged commit cfece2c into main Sep 9, 2022
@gshank gshank deleted the renaming_metric_spec branch September 9, 2022 18:59
This was referenced Sep 12, 2022
Closed
Closed
@jtcohen6 jtcohen6 mentioned this pull request Sep 13, 2022
Merged
5 tasks
josephberni pushed a commit to Gousto/dbt-core that referenced this pull request Sep 16, 2022
@callum-mcdata @josephberni
Renaming Attributes In Metric Spec (dbt-labs#5775)
bc44437 
* making updates - see what fails

* updating tests

* adding timestamp to ok_metric_no_model

* adding changie and fixing description error

* test fixes

* updating schema renderer

* fixing test_yaml_render

* file cleaning and window tests
@popeliam popeliam mentioned this pull request Oct 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gshank gshank gshank approved these changes

@nathaniel-may nathaniel-may Awaiting requested review from nathaniel-may nathaniel-may is a code owner automatically assigned from dbt-labs/core-team

@colin-rogers-dbt colin-rogers-dbt Awaiting requested review from colin-rogers-dbt colin-rogers-dbt was automatically assigned from dbt-labs/core-group

Assignees
No one assigned
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1130] [Feature] Update Metric Spec With Clearer Names
3 participants
@callum-mcdata @gshank @jtcohen6