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.

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

cljx => cljc, no cljsee #426

Merged
merged 34 commits into from Nov 2, 2021
Merged

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Oct 21, 2021

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: frenchy64@d8a5c08
[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:

@frenchy64 frenchy64 changed the title WIP: cljx => cljc conversion WIP: cljx => cljc conversion, no cljsee Oct 21, 2021
@loganlinn loganlinn self-requested a review October 21, 2021 18:03
@loganlinn
Copy link
Member

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

@frenchy64 frenchy64 changed the title WIP: cljx => cljc conversion, no cljsee cljx => cljc conversion, no cljsee Oct 21, 2021
@frenchy64 frenchy64 marked this pull request as ready for review October 21, 2021 18:19
@frenchy64
Copy link
Contributor Author

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

@frenchy64 frenchy64 changed the title cljx => cljc conversion, no cljsee cljx => cljc, no cljsee Oct 21, 2021
@w01fe
Copy link
Member

w01fe commented Oct 21, 2021

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
Copy link
Contributor Author

frenchy64 commented Oct 21, 2021

@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
Copy link
Member

w01fe commented Oct 22, 2021

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
Copy link
Contributor Author

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

@w01fe
Copy link
Member

w01fe commented Oct 23, 2021

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
Copy link
Contributor Author

Yeah, how about we try and merge the build separately? It seems to work as-is #427

@w01fe w01fe closed this Oct 24, 2021
@w01fe w01fe reopened this Oct 24, 2021
@w01fe
Copy link
Member

w01fe commented Oct 24, 2021

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

@w01fe
Copy link
Member

w01fe commented Oct 31, 2021

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!

Copy link
Member

@loganlinn loganlinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the delay. Everything looks great to me! I've tested this out on few other projects without any issue. Thanks again!


#?(:clj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@w01fe w01fe merged commit 2b864ab into plumatic:master Nov 2, 2021
@w01fe
Copy link
Member

w01fe commented Nov 2, 2021

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

@frenchy64
Copy link
Contributor Author

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

@quoll
Copy link

quoll commented Nov 2, 2021

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

@w01fe
Copy link
Member

w01fe commented Nov 3, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cjlx has been deprecated
4 participants