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

feat: support for custom grain #55

Merged
merged 7 commits into from
Oct 21, 2024
Merged

Conversation

serramatutu
Copy link
Collaborator

Summary

This PR adds support for custom grain and deprecates old fields that use the TimeGranularity enum. I also deprecated TimeGranularity itself.

I recommend reviewing commit by commit :)

This commit adds a new `BaseModel.DEPRECATED` key that can be added to
the dataclasses `metadata` dict to indicate that this field is
deprecated.
@serramatutu
Copy link
Collaborator Author

By the way: I had another PR for this that kept the old TimeGranularity enum but that was kinda ugly, so I decided to close it and open this one: #54

This commit introduces a new `DeprecatedMixin` class that can be added
to any model class to mark that class as deprecated. It will throw a
warning if the user tries to instantiate the class.
@serramatutu serramatutu force-pushed the serramatutu/custom-grain-support branch from b98ab02 to 564c332 Compare October 17, 2024 14:54
@@ -8,7 +8,9 @@ kindFormat: '### {{.Kind}}'
changeFormat: '* {{.Body}}'
kinds:
- label: Breaking Changes
auto: major
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we're under 1.0 we can make breaking changes in minor versions so I changed it.

@serramatutu serramatutu force-pushed the serramatutu/custom-grain-support branch from 564c332 to d34961f Compare October 17, 2024 15:11
Copy link

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

The handling for custom grain looks right, but can we add some tests? Not sure if you have e2e tests for this stuff but it would be great to be able to confirm you can send a request with custom grain and get back an expected result

dbtsl/models/dimension.py Outdated Show resolved Hide resolved
dbtsl/models/metric.py Outdated Show resolved Hide resolved
dbtsl/models/saved_query.py Outdated Show resolved Hide resolved
@serramatutu
Copy link
Collaborator Author

serramatutu commented Oct 17, 2024

@courtneyholcomb about testing:

  • we have unit tests that make sure all the GraphQL we would send to the server is valid GraphQL using our schema
  • we have integration tests that make real requests to list metrics, dimensions, saved queries etc (here)
  • i also ran a manual script from examples/ (here) to ensure we pull the correct data.

Since we introduced custom grains, the old `queryable_granularities` is
deprecated in favor of the new `queryable_time_granilarities`, which is
just a simple list of strings. This commit marks the `TimeGranularity`
class and all granularity fields that return it as deprecated.
This commit changes the `OrderByGroupBy` class to use `str` instead of
the deprecated `TimeGranularity` enum as its input grain. We can do this
without a deprecation because we haven't released the SDK since the
order by refactor, so we can just change it.
We need to call `BaseModel._register_subclasses` otherwise models will
fail to use `camelCase` and raise deprecation warnings. That is done in
`dbtsl.models.__init__`. If the user never explicitly imports that, this
won't get called, and they might get an error.

This fixes that by adding an explicit call to it on the library init.
We are raising deprecation warnings from the GQL client when we
instantiate the models. To avoid the warning spam, we filter those
warnings out. They should only be display if the user uses any
deprecated class, not us.
@serramatutu serramatutu force-pushed the serramatutu/custom-grain-support branch from ccd5cde to ed78b15 Compare October 21, 2024 12:36
@serramatutu serramatutu merged commit 7ab6312 into main Oct 21, 2024
8 checks passed
@serramatutu serramatutu deleted the serramatutu/custom-grain-support branch October 21, 2024 12:41
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.

3 participants