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

MLv2: Simplify MetadataProvider protocols (16 methods/3 protocols => 7 methods/2 protocols) #42070

Merged
merged 18 commits into from May 3, 2024

Conversation

camsaul
Copy link
Member

@camsaul camsaul commented Apr 30, 2024

  • Consolidate the 12 methods in the two MetadataProvider and BulkMetadataProvider protocols into 5 methods in a single protocol -- instead of separate methods to fetch a Field/Card/Table/LegacyMetric/Segment as well as a bulk method, just have the metadata providers implement only the bulk method and add helper functions for fetching single objects of different types
  • Remove unused cached-database and store-database! methods from the CachedMetadataProvider protocol
  • Finally finish the cleanup of moving everything away from using vars like lib.metadata/LegacyMetricMetadata to the schema keywords e.g. ::lib.schema.metadata/legacy-metric

@camsaul camsaul requested a review from a team April 30, 2024 20:16
@camsaul camsaul added the no-backport Do not backport this PR to any branch label Apr 30, 2024
Copy link

replay-io bot commented Apr 30, 2024

Status Complete ↗︎
Commit c549aa6
Results
⚠️ 8 Flaky
2447 Passed

@@ -256,6 +268,10 @@
[:description {:optional true} [:maybe ::lib.schema.common/non-blank-string]]])

(mr/def ::legacy-metric
"Malli schema for a legacy v1 [[metabase.models.legacy-metric]], but with kebab-case keys. A Metric defines an MBQL
snippet with an aggregation and optionally a filter clause. You can add a `:metric` reference to the `:aggregations`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be :legacy-metric?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's talking about :metric clauses which are still called :metric AFAIK even if they are V1 metrics

src/metabase/models/segment.clj Outdated Show resolved Hide resolved
@camsaul camsaul enabled auto-merge (squash) May 2, 2024 02:01
@camsaul camsaul merged commit 3829b2c into master May 3, 2024
108 checks passed
@camsaul camsaul deleted the simplified-MetadataProvider-protocol branch May 3, 2024 05:41
Copy link

github-actions bot commented May 3, 2024

@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants