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

metabase.xrays API namespace #42491

Merged
merged 4 commits into from
May 10, 2024
Merged

metabase.xrays API namespace #42491

merged 4 commits into from
May 10, 2024

Conversation

camsaul
Copy link
Member

@camsaul camsaul commented May 9, 2024

  • Consolidate metabase.transforms, metabase.domain-entities, and metabase.automagic-dashboards into a single metabase.xrays module.
  • Remove unused /api/transform endpoint.
  • Some additional dev.deps-graph helpers.

@camsaul camsaul mentioned this pull request May 9, 2024
Open
@metabase-bot metabase-bot bot added the .Team/QueryProcessor(deprecated) Use .Team/Querying instead label May 9, 2024
@camsaul camsaul requested review from johnswanson and a team May 9, 2024 22:21
@camsaul camsaul added the no-backport Do not backport this PR to any branch label May 9, 2024
@replay-io replay.io
Copy link

replay-io bot commented May 9, 2024 •
edited
Loading

Status Complete ↗︎
Commit 6ed5e4e
Results
⚠️ 10 Flaky
✅ 2473 Passed

crisptrutski
crisptrutski approved these changes May 10, 2024
View reviewed changes
.clj-kondo/config.edn
@@ -846,14 +827,14 @@
;; the following patterns are considered to be test namespaces:
;;
;; - Any namespace ending in `-test` or `-test.whatever`
;; - Any namespace ending in `test-util`
;; - Any namespace containing in `test-util`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
;; - Any namespace containing in `test-util`
;; - Any namespace containing `test-util`

dev/src/dev/deps_graph.clj
Comment on lines +77 to +94
(defn circular-dependencies
"Build a graph of module => set of modules it refers to that also refer to this module."
([]
(let [module->deps (module-dependencies)]
(letfn [(circular-dependency? [module-x module-y]
(and (contains? (get module->deps module-x) module-y)
(contains? (get module->deps module-y) module-x)))
(circular-deps [module]
(let [module-deps (get module->deps module)]
(not-empty (into (sorted-set)
(filter (fn [dep]
(circular-dependency? module dep)))
module-deps))))]
(into (sorted-map)
(keep (fn [module]
(when-let [circular-deps (circular-deps module)]
[module circular-deps])))
(keys module->deps)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't matter too much, but you might as well unify the 1-arity variant with the circular-deps helper as they have the same contract and basically the same name too. The overloaded version will avoid a bunch of necessary calculations, and the code will be a bit less nested too.

Haven't checked this refactor for dumb mistakes.

Suggested change
(defn circular-dependencies
"Build a graph of module => set of modules it refers to that also refer to this module."
([]
(let [module->deps (module-dependencies)]
(letfn [(circular-dependency? [module-x module-y]
(and (contains? (get module->deps module-x) module-y)
(contains? (get module->deps module-y) module-x)))
(circular-deps [module]
(let [module-deps (get module->deps module)]
(not-empty (into (sorted-set)
(filter (fn [dep]
(circular-dependency? module dep)))
module-deps))))]
(into (sorted-map)
(keep (fn [module]
(when-let [circular-deps (circular-deps module)]
[module circular-deps])))
(keys module->deps)))))
(defn circular-dependencies
"Build a graph of module => set of modules it refers to that also refer to this module."
([]
(let [module->deps (module-dependencies)]
(into (sorted-map)
(keep (fn [module]
(when-let [circular-deps (circular-dependencies module module->deps)]
[module circular-deps])))
(keys module->deps))))
([module]
(circular-dependencies module (module-dependencies)))
([module module->deps]
(letfn [(circular-dependency? [module-x module-y]
(and (contains? (get module->deps module-x) module-y)
(contains? (get module->deps module-y) module-x)))]
(not-empty (into (sorted-set)
(filter (fn [dep]
(circular-dependency? module dep)))
(get module->deps module))))))

test/metabase/xrays/test_util/automagic_dashboards.clj
@@ -9,7 +9,7 @@
[metabase.util :as u]
[metabase.util.malli.schema :as ms]))

(defmacro with-dashboard-cleanup
(defmacro with-dashboard-cleanup!
Copy link
Contributor

Choose a reason for hiding this comment

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

Kudos for marking all these helpers

src/metabase/api/transform.clj
@@ -1,31 +0,0 @@
(ns metabase.api.transform
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels good man 🐸

@crisptrutski crisptrutski mentioned this pull request May 10, 2024
Merged
@camsaul
Copy link
Member Author

camsaul commented May 10, 2024

Thanks for the suggestions @crisptrutski, I'll change this stuff in the next PR in the series which has changes to the deps graph stuff anyway

@camsaul camsaul merged commit 59e7b3d into master May 10, 2024
111 checks passed
@camsaul camsaul deleted the xrays-api-namespace branch May 10, 2024 16:14
@github-actions GitHub Actions
Copy link
Contributor

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

uladzimirdev pushed a commit that referenced this pull request May 11, 2024
@camsaul @uladzimirdev
metabase.xrays API namespace (#42491)
68cfe7d 
* Remove unused `metabase.api.transform`

* Copy the deps graph improvements from my other PR

* New combined X-Rays API namespace

* Fix kondo error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crisptrutski crisptrutski crisptrutski approved these changes

@johnswanson johnswanson Awaiting requested review from johnswanson

Assignees

@camsaul camsaul

Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor(deprecated) Use .Team/Querying instead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@camsaul @crisptrutski