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

update database listing endpoint to account of org.inherits #299

Merged
merged 3 commits into from
Apr 1, 2015
Merged

update database listing endpoint to account of org.inherits #299

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97b8368
update database listing endpoint so that if the org 'inherits' we ret…
agilliland Apr 1, 2015
925d69f
more concise way to write our query.
agilliland Apr 1, 2015
72f7d5e
formatting.
agilliland Apr 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/metabase/api/meta/db.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@
"Fetch all `Databases` for an `Org`."
[org]
{org Required}
(read-check Org org)
(-> (sel :many Database :organization_id org (order :name))
(hydrate :organization)))
(let-404 [{:keys [id inherits] :as org} (sel :one Org :id org)]
(read-check org)
(-> (sel :many Database (order :name) (where (if inherits {}
{:organization_id id})))
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky but it looks like your if is indented weird.
This is the normal way to do it in Clojure:

;; then clause on newline
(if cond
  :then
  :else)

;; then clause on same line
(if cond :then
    :else)

😿

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm. i was sure that i read somewhere if you write it the second way that the :then and :else should be aligned. in any case, i don't particularly care for that styling, so i'll stick with the first option which i think is the more readable approach

Copy link
Member

Choose a reason for hiding this comment

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

I find the same line format more readable in cases like this:

(if field-aggregation? (-> (sel :one [Field :base_type :special_type]
                                :id (-> query :query :aggregation second)) 
                           (select-keys [:base_type :special_type]))
    (case aggregation-type                                                 
      :count {:base_type "IntegerField"                                    
              :special_type "number"}))

vs

(if field-aggregation?
  (-> (sel :one [Field :base_type :special_type]
           :id (-> query :query :aggregation second)) 
      (select-keys [:base_type :special_type]))
  (case aggregation-type                                                 
    :count {:base_type "IntegerField"                                    
            :special_type "number"}))

Because at a glance it's a lot clearer which is then and which is else because they have very different indentation levels.

FWIW I find the way it's indented in Emacs Lisp the most readable:

(if cond
    :then
  :else)

But it makes sense to follow Clojure conventions.

(hydrate :organization))))

(defendpoint POST "/"
"Add a new `Database` for `Org`."
Expand Down