-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Evaluate all defsetting arguments at runtime #38128
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
|
src/metabase/models/setting.clj
Outdated
{:description-form ~description-form}))) | ||
|
||
;; This exists as its own method so that we can stub it in tests | ||
(defn- in-test? [ns] (str/ends-with? ns "-test")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(defn- in-test? [ns] (str/ends-with? ns "-test")) | |
(defn- ns-in-test? [ns] (str/ends-with? ns "-test")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things:
- I think we should prefix the name with
ns
, it was fine before if it does not take an argument - we should call
ns-name
onns
because I usually think when we seens,
we think of it as a symbol, not str. So we should avoid that footgun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It feels non-idiomatic to repeat the name of the arg in the fn name for a predicate, but don't feel strongly.
- The ns here is a symbol, which
str/end-with?
implicitly handles converting to a string. I think you meant you expect aclojure.lang.Namespace
object? I want this method to "just work" with settings definitions, where we've already turned that into data (a symbol) by callingns-name
. I'll rename the argument itself tons-name
, which I think also removes my discomfort in (1) 😆
bb72594
to
88f0c8d
Compare
{:description-form ~description-form}))) | ||
|
||
;; This exists as its own method so that we can stub it in tests | ||
(defn- ns-in-test? [ns-name] (str/ends-with? ns-name "-test")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shower thought - instead of stubbing this we could cover its logic by switching namespaces within the test. Not sure which is the lesser evil 🙂
88f0c8d
to
c9c094f
Compare
c9c094f
to
9b8330e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@crisptrutski Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Description
This refactor changes
defsetting
so that we only process its arguments at runtime, allowing us to use references and expressions to define things dynamically.Note that this PR does not add support for a trailing kwargs map, that's left in #38090, which I plan to rebase on this branch.Scrapped that feature anyway.