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

Code review #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Code review #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/dove/core.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ Represents an amount of time defined by a number of months, days and
`fixed(12)` in generated Java sources."
(->avro-fixed? 12))

;; CR: be careful with memoize. It might explode the memory!
(def enum-obj-spec-value
(memoize
(fn [enum-class spec-values]
Expand Down Expand Up @@ -282,6 +283,7 @@ Represents an amount of time defined by a number of months, days and
:optional
:required))))

;; CR: separate the code for each schema into its own expression
(extend-protocol ToSpec
Schema$StringSchema
(to-spec! [this args]
Expand Down Expand Up @@ -349,7 +351,7 @@ Represents an amount of time defined by a number of months, days and
(do (spec-def args `~spec-keyword)
(keyword (:spec-ns args) (:spec-name args)))
(keyword (.getNamespace this) (.getName this)))))

;; CR: create a function ->keyword that receives an avro obj and returns the corresponding keyword
Schema$FixedSchema
(to-spec! [this args]
(let [spec-keyword (keyword (.getNamespace this)
Expand All @@ -363,6 +365,7 @@ Represents an amount of time defined by a number of months, days and
:spec-ns (.getNamespace this)
:spec-name (.getName this))
`~(->avro-fixed? (.getFixedSize this))))
;; CR: this code appears several time - can you make a function of it?
(if (and (:spec-ns args)
(:spec-name args))
(do (spec-def args `~spec-keyword)
Expand Down Expand Up @@ -407,6 +410,7 @@ Represents an amount of time defined by a number of months, days and
Schema$RecordSchema
(to-spec! [this args]
(let [record-fields (.getFields this)
;; CR: refactor to [spec-ns spec-name spec-keyword] (spec-data this): 3 lines combine into 1 :)
spec-ns (.getNamespace this)
spec-name (.getName this)
spec-keyword (keyword spec-ns spec-name)
Expand All @@ -417,9 +421,11 @@ Represents an amount of time defined by a number of months, days and
(update acc (optional-key? field args) conj spec-key)))
{}
record-fields)]
;; CR: something is weird to-spec! seems to be a function that returns something but its return value is ignored
(doseq [field record-fields]
(to-spec! field (assoc args
:spec-ns (hierarchy-derive spec-ns spec-name))))
;; CR: does spec-def have side effect? if yes rename to spec-def!
(spec-def (assoc args
:spec-ns spec-ns
:spec-name spec-name)
Expand All @@ -430,6 +436,7 @@ Represents an amount of time defined by a number of months, days and
(if (and (:spec-ns args)
(:spec-name args))
(do
;; can you make spec-def return the keyword that corresponds to the spec. It will save the need for the `do` expression
(spec-def args `~spec-keyword)
(keyword (:spec-ns args) (:spec-name args)))
spec-keyword))))
Expand Down