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

Add support for spec-2 #163

Merged
merged 9 commits into from
Sep 29, 2022
Merged

Add support for spec-2 #163

merged 9 commits into from
Sep 29, 2022

Conversation

r0man
Copy link
Contributor

@r0man r0man commented Sep 13, 2022

Hello,

this PR adds support for spec-2 to orchard. spec-2 is supposed to be the successor of clojure.spec.alpha, but is still in an experimental state. The main advantage of spec-2 is that it adds s/schema and s/select which should improve some short comings of s/keys from the original spec. s/schema and s/select are supposed to help with re-using spec declarations. You would use s/schema to define the general shape of a data structure and then use s/select to make concrete sub selections of specs that have been defined with s/schema. It is not recommended to use spec-2 in production, but some people might do.

When Clojure Spec was initially released it lived under the namespace clojure.spec. Some time after the release it was renamed to clojure.spec.alpha. I believe because of this transition phase Orchard supports both namespaces and checks at run time which version of Clojure Spec is available and resolves specs accordingly. This PR adds a third option to the mix.

The PR changes Orchard to support spec-list, spec-form and spec-example NREPL ops in the following way:

For the spec-form and spec-example NREPL ops the Clojure specs are resolved and generated in the following order:

  • spec-alpha-2 (aka clojure.alpha.spec)

  • spec-alpha (aka clojure.spec.alpha, which is shipped with Clojure these days)

  • spec (aka clojure.spec, which is shipped in old Clojure versions?)

There is however a chance of a conflict if a spec is declared in more than one of the registries, and multiple variants of Clojure Spec are loaded at the same time. The most likely scenario is that people who use spec-2 have loaded both spec-alpha-2 (aka clojure.alpha.spec) and spec-alpha (aka clojure.spec.alpha). So spec-2 specs would shadow specs defined in a "previous" spec variant.

For the spec-list NREPL op the available spec registries are merged with spec-2 (clojure.alpha.spec) taking precedence over spec-alpha (aka clojure.spec.alpha) over spec (aka clojure.spec).

This order has been chosen because I believe people using spec-2 would prefer to see those specs instead of specs form older versions, in the unlikely case of a spec defined in multiple registries.

To be honest, I'm not super confident of the "unlikely" case and what happens in the future. This was the most pragmatic approach I could think of to get spec-2 working in Cider. It seems to work in my project and it should not affect people not using spec-2.

An alternative approach could be:

We could index the results of the NREPL operations by their spec variants. So, the spec-list op could return:

{:spec-alpha-2 ["some/spec", "another/spec"]
:spec-alpha ["some/spec"]
:spec ["some/spec"]}

And something similar for the other ops. The Cider frontend could then display all available Specs at the same time. If we want to go this direction we should probably introduces a new middleware with new ops for this, or add additional keys to the result of the existing ops. This might be the more "clean" approach, to support multiple flavors of Clojure Spec. But I'm not sure if it's worth the effort and I believe it would complicate the frontends a bit. Also I don't know how important it is to still support clojure.spec.

Any other ideas how to best integrate this?

Another open question is:

If we want to add tests for spec-2 we somehow need to bring the dependency in. spec-2 is only available via deps.edn. We could try to add it to orchard via lein-tools-deps or use some script to fetch it and leverage Leinigen's checkout feature to somehow add it to the classpath.

WDYT?

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the changelog (if adding/changing user-visible functionality)

@r0man
Copy link
Contributor Author

r0man commented Sep 13, 2022

Here are the changes I made to Cider:
https://github.com/r0man/cider/tree/spec2

It adds pretty printing for s/schema and s/select and seems to work fine with the approach of this PR.

@bbatsov
Copy link
Member

bbatsov commented Sep 19, 2022

If we want to add tests for spec-2 we somehow need to bring the dependency in. spec-2 is only available via deps.edn. We could try to add it to orchard via lein-tools-deps or use some script to fetch it and leverage Leinigen's checkout feature to somehow add it to the classpath.

Can you elaborate on this? Why is it hard to have it as a dep with a Lein project?

@bbatsov
Copy link
Member

bbatsov commented Sep 19, 2022

Overall, I'm fine with the approach you've taken in this PR, but I have to admit that the current situation with the 3 versions of Spec is killing me. Let's hope at the end it will be just Spec 2 and nothing else. I find this to be extremely funny:

spec-alpha-2 (aka clojure.alpha.spec)

spec-alpha (aka clojure.spec.alpha, which is shipped with Clojure these days)

Naming is hard!

@vemv
Copy link
Member

vemv commented Sep 19, 2022

We could try to add it to orchard via lein-tools-deps or use some script to fetch it and leverage Leinigen's checkout feature to somehow add it to the classpath.

It's actually super easy - one clones the repo (shallowly, of course) and adds "src/main/clojure" to the Lein :source-paths.

(resolve (symbol "clojure.spec.gen" ~fname)))]
(f# ~@args)))
(defn- spec [sym & args]
(when-let [f (resolve sym)]
Copy link
Member

Choose a reason for hiding this comment

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

Let's try requireing before resolveing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the call-when-resolved macro we introduced for the datafy code. So now we only require/resolve once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit more verbose though :/

(f# ~@args)))
(defn- spec [sym & args]
(when-let [f (resolve sym)]
(try (apply f args) (catch Exception _))))
Copy link
Member

Choose a reason for hiding this comment

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

This try/catch wasn't here before and doesn't seem a great idea to me...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did this because spec-2 is not as relaxed as the others when it comes to what types of arguments you pass to it's functions. This has been moved to the individual spec fn wrappers where I tried to mirror the behavior of the current version of spec, even though we are looking up in 3 different places.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; This are all wrappers of clojure.spec.[alpha] functions. ;;
;; We can't simply require the ns because it's existence depends on ;;
;; clojure version ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(defn get-spec [v] (spec "get-spec" v))
Copy link
Member

Choose a reason for hiding this comment

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

Probably the above comment should be extended to mention Spec 2 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@r0man r0man mentioned this pull request Sep 19, 2022
6 tasks
@r0man
Copy link
Contributor Author

r0man commented Sep 19, 2022

Here's the PR for CIDER clojure-emacs/cider#3249

@r0man
Copy link
Contributor Author

r0man commented Sep 19, 2022

If we want to add tests for spec-2 we somehow need to bring the dependency in. spec-2 is only available via deps.edn. We could try to add it to orchard via lein-tools-deps or use some script to fetch it and leverage Leinigen's checkout feature to somehow add it to the classpath.

Can you elaborate on this? Why is it hard to have it as a dep with a Lein project?

I think this is a non-issue. :) I did what @vemv suggested. I'm checking out spec-2 via git in the Makefile and added the source dir to project.clj. I had to exclude those sources from eastwood, since it detected some lint issues in spec-2.

@bbatsov bbatsov merged commit 25c1823 into clojure-emacs:master Sep 29, 2022
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.

3 participants