Grasp doesn't seem to descend into macro forms #28
Comments
dpsutton
added a commit
to metabase/metabase
that referenced
this issue
Jun 7, 2022
import wasn't necessary
forgot to check the driver sources for i18n
for some reason grasp doesn't descend into
```clojure
(defmacro ^:private deffingerprinter
[field-type transducer]
{:pre [(keyword? field-type)]}
(let [field-type [field-type :Semantic/* :Relation/*]]
`(defmethod fingerprinter ~field-type
[field#]
(with-error-handling
(with-global-fingerprinter
(redux/post-complete
~transducer
(fn [fingerprint#]
{:type {~(first field-type) fingerprint#}})))
(trs "Error generating fingerprint for {0}" (sync-util/name-for-logging field#))))))
```
I've opened an issue on [grasp](borkdude/grasp#28)
|
I can look at this hopefully when I have some time on the train to ClojureD this week, else next week, but a minimal repro would surely help to look into this faster. |
|
I think I found the issue. When reading, a macro looks like this: and this is the form that grasp "grasps" against. So it is the fully expanded but unevaluated form that the syntax quote gives back. |
|
ah so |
|
@dpsutton This is what the unevaluated form looks like, basically. Always evaluating what comes back from the reader won't work/isn't safe. To find stuff in syntax-quote expression I suggest inspecting them in a REPL: and then making your spec account for that. We could document this. |
dpsutton
added a commit
to metabase/metabase
that referenced
this issue
Jun 8, 2022
* Create po template file (pot) from clojure Rather than use xgettext we can use grasp to look for translation sites. The reason to do this is twofold: Multiline Translated Strings ---------------------------- We can use multiline strings in source to aide in readability. This [PR](#22901) was abandoned because we xgettext cannot be expected to combine string literals into a single string for translation purposes. But we can certainly do that with clojure code. ```clojure (defn- form->string-for-translation "Function that turns a form into the translation string. At the moment it is just the second arg of the form. Afterwards it will need to concat string literals in a `(str \"foo\" \"bar\")` situation. " [form] (second form)) (defn- analyze-translations [roots] (map (fn [result] (let [{:keys [line _col uri]} (meta result)] {:file (strip-roots uri) :line line :message (form->string-for-translation result)})) (g/grasp roots ::translate))) ``` `form` is the literal form. So we can easily grab all of the string literals out of it and join them here in our script. The seam is already written. Then reviving the PR linked earlier would upgrade the macros to understand that string literals OR `(str <literal>+)` are acceptable clauses. Translation context ------------------- Allowing for context in our strings. The po format allows for context in the file format. ``` msgctxt "The update is about changing a record, not a timestamp" msgid "Failed to notify {0} Database {1} updated" msgstr "" ``` See [this issue](#22871 (comment)) for an example situation. This wouldn't help in this particular instance because it is on the Frontend though. But we could have a format like ```clojure (trs "We" (comment "This is an abbreviation for Wednesday, not the possessive 'We'")) ``` The macro strips out the `comment` form and we can use it when building our pot file. Note there is a difficulty with this though since all source strings must be unique. There can be multiple locations for each translated string. ``` ,#: /metabase/models/field_values.clj:89 ,#: /metabase/models/params/chain_filter.clj:588 msgid "Field {0} does not exist." msgstr "" ``` The leading commas are present to prevent commit message comments. But if one location has a context and the other doesn't, or even worse, if they have two different contexts, we have a quandry: we can only express one context. Probably easy to solve or warn on, but a consideration. Caught Errors ------------- The script blew up on the following form: ```clojure (-> "Cycle detected resolving dependent visible-if properties for driver {0}: {1}" (trs driver cyclic-props)) ``` No tooling could (easily) handle this properly. Our macro assertions don't see the thread. But xgettext never found this translation literal. I warn in the pot generation so we can fix this. We could also have a test running in CI checking that all translations are strings and not symbols. Fundamental Tool ---------------- The sky is the limit because of this fundamental grasp tool: ```clojure enumerate=> (first (g/grasp single-file ::translate)) (trs "Failed to notify {0} Database {1} updated" driver id) enumerate=> (meta *1) {:line 35, :column 22, :uri "file:/Users/dan/projects/work/metabase/src/metabase/driver.clj"} ``` We can find all usages of tru/trs and friends and get their entire form and location. We can easily do whatever we want after that. Verifying Translation scripts still work ---------------------------------------- You can check a single file is valid with `msgcat <input.pot> -o combined.pot`. This will throw if the file is invalid. The general script still works: ``` ❯ ./update-translation-template [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs.pprint.js as it exceeds the max of 500KB. [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs.core.js as it exceeds the max of 500KB. [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs-runtime/cljs.core.js as it exceeds the max of 500KB. Warning: environ value /Users/dan/.sdkman/candidates/java/current for key :java-home has been overwritten with /Users/dan/.sdkman/candidates/java/17.0.1-zulu/zulu-17.jdk/Contents/Home SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details. Created pot file at ../../locales/metabase-backend.pot #<----- new line here Warning: environ value /Users/dan/.sdkman/candidates/java/current for key :java-home has been overwritten with /Users/dan/.sdkman/candidates/java/17.0.1-zulu/zulu-17.jdk/Contents/Home 2022-06-06 15:05:57,626 INFO metabase.util :: Maximum memory available to JVM: 8.0 GB Warning: protocol #'java-time.core/Amount is overwriting function abs WARNING: abs already refers to: #'clojure.core/abs in namespace: java-time.core, being replaced by: #'java-time.core/abs WARNING: abs already refers to: #'clojure.core/abs in namespace: java-time, being replaced by: #'java-time/abs 2022-06-06 15:06:01,368 WARN db.env :: WARNING: Using Metabase with an H2 application database is not recommended for production deployments. For production deployments, we highly recommend using Postgres, MySQL, or MariaDB instead. If you decide to continue to use H2, please be sure to back up the database file regularly. For more information, see https://metabase.com/docs/latest/operations-guide/migrating-from-h2.html 2022-06-06 15:06:03,594 INFO util.encryption :: Saved credentials encryption is DISABLED for this Metabase instance. 🔓 For more information, see https://metabase.com/docs/latest/operations-guide/encrypting-database-details-at-rest.html WARNING: abs already refers to: #'clojure.core/abs in namespace: taoensso.encore, being replaced by: #'taoensso.encore/abs WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.math, being replaced by: #'kixi.stats.math/abs WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.test, being replaced by: #'kixi.stats.math/abs WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.distribution, being replaced by: #'kixi.stats.math/abs msgcat: msgid '{0} metric' is used without plural and with plural. msgcat: msgid '{0} table' is used without plural and with plural. ``` I'm not sure what the last two lines are about but I suspect they are preexisting conditions. their form from the final combined pot file (althrough again with leading commas on the filenames to prevent them from being omitted from the commit message) ``` ,#: frontend/src/metabase/admin/permissions/components/PermissionsConfirm.jsx:32 ,#: src/metabase/automagic_dashboards/core.clj ,#: target/classes/metabase/automagic_dashboards/core.clj ,#, fuzzy, javascript-format msgid "{0} table" msgid_plural "{0} tables" msgstr[0] "" "#-#-#-#-# metabase-frontend.pot #-#-#-#-#\n" "#-#-#-#-# metabase-backend.pot (metabase) #-#-#-#-#\n" msgstr[1] "#-#-#-#-# metabase-frontend.pot #-#-#-#-#\n" ... ,#: frontend/src/metabase/query_builder/components/view/QuestionDescription.jsx:24 ,#: src/metabase/automagic_dashboards/core.clj ,#: target/classes/metabase/automagic_dashboards/core.clj ,#, fuzzy, javascript-format msgid "{0} metric" msgid_plural "{0} metrics" msgstr[0] "" "#-#-#-#-# metabase-frontend.pot #-#-#-#-#\n" "#-#-#-#-# metabase-backend.pot (metabase) #-#-#-#-#\n" msgstr[1] "#-#-#-#-# metabase-frontend.pot #-#-#-#-#\n" ``` * Add drivers, one override, remove unused import import wasn't necessary forgot to check the driver sources for i18n for some reason grasp doesn't descend into ```clojure (defmacro ^:private deffingerprinter [field-type transducer] {:pre [(keyword? field-type)]} (let [field-type [field-type :Semantic/* :Relation/*]] `(defmethod fingerprinter ~field-type [field#] (with-error-handling (with-global-fingerprinter (redux/post-complete ~transducer (fn [fingerprint#] {:type {~(first field-type) fingerprint#}}))) (trs "Error generating fingerprint for {0}" (sync-util/name-for-logging field#)))))) ``` I've opened an issue on [grasp](borkdude/grasp#28) * Use vars rather than name based matching
|
I wonder if this is a good case for using rewrite-clj to allow descending into unexpanded syntax quotes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm looking for all strings in our codebase at Metabase that need translations. To this end i'm doing
(I only just now realized I can resolve symbols for smarter matching but not relevant here). This spec works great and finds almost all usages. However I've found one that it does not find:
I'm going to try to come up with a minimal reproduction because there's a lot going on.
Note I'm not expecting to find the usage at macro invocations and call sites, just in the literal form here which I would expect it to be able to descend into and match on the
trs.The text was updated successfully, but these errors were encountered: