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

Add Conversion Metrics Properties #209

Merged
merged 9 commits into from
Nov 29, 2023
Merged

Conversation

WilliamDee
Copy link
Contributor

@WilliamDee WilliamDee commented Nov 16, 2023

Description

  • Added protocols/implementations for conversion metrics properties
  • Added transformation step for input measures for conversion metrics
  • Added parser updates to handle parsing conversion metrics properties

Spec

Reference #210

Checklist

@cla-bot cla-bot bot added the cla:yes label Nov 16, 2023
Copy link

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.

@WilliamDee WilliamDee force-pushed the will/add-conversion-metric branch from 549313e to 8c2f986 Compare November 26, 2023 20:06
@WilliamDee WilliamDee force-pushed the will/add-conversion-metric branch from 8c2f986 to 30890ae Compare November 26, 2023 20:27
@WilliamDee WilliamDee force-pushed the will/add-conversion-metric branch from 30890ae to b93ac79 Compare November 26, 2023 20:31
@WilliamDee WilliamDee marked this pull request as ready for review November 27, 2023 17:46
Copy link
Collaborator

@tlento tlento left a comment

Choose a reason for hiding this comment

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

I think there's a general issue of too many accessors, and I think we'll be better off starting with more streamlined protocols. In particular, I'd like to avoid the get_property_or_error accessor pattern you've introduced here.

Otherwise, this looks good. We can see if @QMalcolm has any added feedback while we clean up the accessors.

dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

This is looking really good! Nothing major from me. @tlento has raised some really good points which I'd also like to see addressed 🙂

dbt_semantic_interfaces/parsing/schemas.py Show resolved Hide resolved
tests/test_implements_satisfy_protocols.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@QMalcolm QMalcolm 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 to me! Thank you for doing this work 🙂

@WilliamDee WilliamDee merged commit bf39313 into main Nov 29, 2023
10 checks passed
@WilliamDee WilliamDee deleted the will/add-conversion-metric branch November 29, 2023 19:47
WilliamDee added a commit that referenced this pull request Dec 7, 2023
### Description
Backporting conversion metric support PRs to `0.4.latest`
- #221
- #209
<!---
Describe the Pull Request here. Add any references and info to help
reviewers
  understand your changes. Include any tradeoffs you considered.
-->

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [x] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants