plumatic / schema

Clojure(Script) library for declarative data description and validation
Other
2.4k stars 256 forks source link

cljx => cljc, no cljsee #426

Closed frenchy64 closed 2 years ago

frenchy64 commented 2 years ago

Close #425

Migrate cljx to cljc, which breaks support for Clojure 1.6.

Reviewers

It is probably safe to hide whitespace changes, since some indentation changes were necessary.

There are several critical typos to look for:

  1. #(:clj ...) instead of #?(:clj ...)
    • the former should never occur (missing ?)
  2. #+clj a b c should be converted to #?(:clj a) b c
    • NOT #?(:clj a b c)
  3. Make sure #+clj corresponds to #?(:clj), ditto for cljs
    • tags should be preserved

Any code that should not be compiled in js contexts in a cljc should be wrapped #?(:clj) (eg., defmacro, and macro helpers).

Build

Added GitHub Actions matrix build to verify, see results on my fork (tests JVM 8+11+17).

Can remove if not wanted, as I know CircleCI is set up (but does not have a matrix build, or pr builds enabled).

Temporary deployment

Deployed jar at this commit: https://github.com/frenchy64/schema/commit/d8a5c08da87529ab9b01349f7dc3a2e6d43f9294 [org.clojars.frenchy64/schema "1.1.13-20211021.171347-1"]

How to try:

Leiningen (example)

  :exclusions [prismatic/schema] ;; global exclusion
  :dependencies [[org.clojars.frenchy64/schema "1.1.13-20211021.171347-1"]]

Clojure CLI (commit)

  ;; deps.edn does not have a global override, git deps might be more reliable. if you use
  ;; the org.clojars.frenchy64/schema jar, inspect `clj -Stree` to ensure `prismatic/schema` is properly excluded.
  :deps {prismatic/schema {:git/url "https://github.com/frenchy64/schema.git"
                           :git/sha "ae728e11d9ced8756894a84fa3096826de73755d"}}

Tasks:

loganlinn commented 2 years ago

This is awesome. I'm happy to help review once ready!

frenchy64 commented 2 years ago

@loganlinn ok ready! Do you have the power (and inclination) to enable the Actions matrix build for this PR?

w01fe commented 2 years ago

I think he does, but I'm also happy to do it. If so can you give more precise instructions please? (I haven't used Actions yet).

frenchy64 commented 2 years ago

@w01fe there are various knobs depending on what you're comfortable with here.

My opinion: For an open source project with no secrets, it should be ok to Allow all Actions, Require approval for first-time contributors who are new to GitHub. This crucially allows PR's from forks to use the build.

If a secret is added, it is never available to forks, so the build will just fail on forks if secrets are absolutely required. And builds are free for open source projects.

w01fe commented 2 years ago

I believe it's enabled now, let me know if that did the trick or not.

I think they were already enabled for contributors, so I'm not sure why it wasn't working (or if this actually fixed anything).

frenchy64 commented 2 years ago

Didn't seem to work, it's possible we need to merge the build first?

w01fe commented 2 years ago

Changes look good to me, thanks for taking this on! I guess the actions might not work until something is on main? Since you've tested on your side, I'm happy to merge and address any issues in a follow-up, or to pull just the actions into main first (although I assume they would fail as-is on the old cljx version?). What do you suggest?

frenchy64 commented 2 years ago

Yeah, how about we try and merge the build separately? It seems to work as-is https://github.com/plumatic/schema/pull/427

w01fe commented 2 years ago

Merging the other then closing and reopening seems to have done the trick, thanks!

w01fe commented 2 years ago

Sorry for lagging on this, I was out of town and waiting to see if @loganlinn was going to review. I looked it over and don't see any issues, so if I we don't hear from him in the next day or two I'll go ahead and merge. Thanks!

w01fe commented 2 years ago

Thanks a bunch for doing this! I can cut a release if it would be helpful, let me know.

frenchy64 commented 2 years ago

@w01fe yes please, I think this is worth a release.

quoll commented 2 years ago

Tested this on our code at work, with complete success.

w01fe commented 2 years ago

Released 1.2.0, please let me know if you run into any issues.