-
Notifications
You must be signed in to change notification settings - Fork 35
Improve release automation. #466
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
base: master
Are you sure you want to change the base?
Conversation
Includes automation for synchronizing the project version and CTIM schema version.
a4c8db8 to
805d813
Compare
|
|
|
@frenchy64 Addressed all your comments. Thanks! |
| #?(:clj [ctim.lib.generators :as gen]) | ||
| [ctim.lib.predicates :as pred] | ||
| [ctim.schemas.vocabularies :as v] | ||
| #?(:clj [ctim.version :refer [ctim-version]]) |
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.
This might work as-is in cljs. https://clojurescript.org/guides/ns-forms#_implicit_refer
Try removing the reader conditionals. If not, maybe we need to bump our cljs version.
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.
Okay, so this doesn't work either. I think I understand why both this and :include-macros true don't work now.
Why ':include-macros true' doesn't work
Given an ns defined in file bar.clj (note extension .clj)
Given a file foo.cljs(note extension .cljs) with the following content:
(ns foo (:require [bar :include-macros true]))
desugars to:
(ns foo (:require-macros [bar])
(:require [bar]))
The presence of both :require and :require-macros causes the cljs compiler to look for both clj and cljs namespaces foo, but there's only the clojure one.
Why implicit refer doesn't work
IIUC, the implicit refer also only works for scenarios where there is both a clj and cljs ns (with the additional constraint that the cljs dependency ns declares :require-macros on the clj ns of the same name).
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.
So, given that the macro is defined in a plain clj ns and that we are working in a cljc file where both of the resulting clj and cljs namespaces depend on the same macro, it is necessary for the cljs ns to declare a dependency on the macro ns using only :require-macros. And the only way to achieve that (I think) is for the cljc file to have separate :clj and :cljs conditionals.
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.
Another possibility is doing it all in the same file:
(ns ctim.schemas.common
#?(:cljs (:require-macros [ctim.schemas.common :refer [ctim-version]]))
...
#?(:clj
(defmacro ctim-version
"This is a macro to support cljs compile-time inlining of the version string
from the JVM resource."
[]
(slurp (io/resource "ctim/version.txt"))))
(def ctim-schema-version (ctim-version))
Which I actually don't hate. It looks a little convoluted. But I'm not sure its worse than the separate file solution. If anything it's just making the quirks easier to see.
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.
I meant I think there's a way to avoid reader conditionals and :require-macros altogether.
i.e., instead of
(ns ctim.schemas.common
#?(:cljs (:require-macros [ctim.version :refer [ctim-version]]))
(:require [ctim.version :refer [ctim-version]]))try
(ns ctim.schemas.common
(:require [ctim.version :refer [ctim-version]]))or
(ns ctim.schemas.common
(:require [ctim.version :as cv]))Failing all that, this might work:
(ns ctim.schemas.common
(#?(:clj :require :cljs :require-macros) [ctim.version :refer [ctim-version]]))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.
Yeah, I had tried your first suggestion. That's what I'm calling "implicit refer" (term is from that clojurescript doc) in my message above.
Your second suggestion does work fine, let's do it. I didn't realize it would let you have multiple :requires.
| (:require [clj-momo.lib.clj-time.coerce :refer [to-long]] | ||
| #?(:clj [clojure.java.io :as io]) | ||
| [clojure.set :refer [map-invert]] | ||
| [clojure.string :as str] |
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.
Can remove requires of clojure.java.io and clojure.string.
| ["vcs" "tag" "--no-sign"] | ||
| ["deploy" "clojars"] | ||
| ["change" "version" "leiningen.release/bump-version"] | ||
| ["sync-schema-version"] |
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.
I think we always want to update the docs when we bump versions, and vice-versa.
Please redefine the "doc" alias to do the equivalent of running ctim.change-schema-version/-main followed by ctim.document/-main.
| # Patch version releases (minor and major increments require a manual edit of version). | ||
| # Expect this task to modify project files and deploy to Clojars. | ||
| # See `release-tasks` in project.clj for details. | ||
| lein release |
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.
This is already recommended a few lines down. Could you consolidate them and separate the instructions so it doesn't look like you should execute them blindly top-to-bottom? I'm often in a hurry when reading this section.
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.
Yeah, I'll work on this tomorrow morning when my brain is fresh. I think there is more to do here too, such as warn about the mistakes I made when doing my release 😃 .
Made these changes in consultation with @frenchy64.
resources/version.edn.resources/version.ednin release automation to synchronize with lein project version.scripts/release.shfile (at least I'm pretty sure it's not used anymore) and update README doc.