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

Make spec-coerce an optional dependency #18

Merged
merged 6 commits into from
Sep 21, 2022
Merged

Make spec-coerce an optional dependency #18

merged 6 commits into from
Sep 21, 2022

Conversation

thumbnail
Copy link
Member

@thumbnail thumbnail commented Sep 16, 2022

Brief

✨ Discussion piece ✨

spec-coerce isn't a common dependency for us, so i'd like to make it optional. Either by excluding the dependency in consumers (opt-out) or adding the dependency by consumers who want to use it (opt-in) or removing it altogether. In this PR i make coerce-map-indicating-invalidity opt-in.

Alternative 1: keep spec-coerce in project.clj and exclude in consumers (made possible by the 'requiring-resolve'-call).
Alternative 2: Remove function, bump major to 2.0.0

context:

WDYT @nedap/sts-clojure-dev ?

QA plan

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Modular design
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have reviewed the PR
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Modular design
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

@thumbnail
Copy link
Member Author

Given that the cljs testsuite broke, a major bump and removing the function all-together may be the path of least resistance.

@vemv
Copy link
Contributor

vemv commented Sep 17, 2022

Perhaps this would work?

;; defined in a separate .clj file
(defmacro when-spec-coerce-available [& body]
  (when (try
          (require 'spec-coerce.core)
          true
          (catch Exception _
            false))
    (do
      ~@body)))

;; no reader conditional needed
(when-spec-coerce-available
  (defn coerce-map-indicating-invalidity ,,,))

Then you'd remove the requires entirely (because there are no conditional requires in cljs) - it would become up to the consumers to 1) provide the dep, and 2) declare the requires

@thumbnail
Copy link
Member Author

because there are no conditional requires in cljs

I thought this was fixed (https://anmonteiro.com/2016/10/clojurescript-require-outside-ns/) but with little cljs experience i'm probably mistaken.

Regarding the when-spec-coerce-available, we do a similar thing in f-s, https://github.com/nedap/formatting-stack/blob/541ffcf6341735a63ca451d4d8782f2feaf7aacb/src/formatting_stack/strategies.clj#L211-L228

It could even be a function, I think 🤔.

But this would still be a breaking change (consumers are forced to change their usage by adding a dep + require).

@vemv
Copy link
Contributor

vemv commented Sep 19, 2022

I thought this was fixed (https://anmonteiro.com/2016/10/clojurescript-require-outside-ns/)

It works only in the repl or similar environments, AFAIK

But this would still be a breaking change

Some breaking changes are better than others :) one has a migration path, the other not. It seems to play well with semver + a changelog

@thumbnail
Copy link
Member Author

one has a migration path, the other not

IMO the migration paths are similar. Adding the require + dependency to your project.clj vs moving the function into your project.

From what I can see in the github search / grep.app there's no (public!) usage of this fn anyway. Removing it likely has very little impact whichever option we pick.

@vemv
Copy link
Contributor

vemv commented Sep 19, 2022

I like using the affected fn for quick web projects

@thumbnail thumbnail force-pushed the spec-coerse.core branch 5 times, most recently from 2bcfb9c to af83583 Compare September 19, 2022 10:04
@thumbnail thumbnail marked this pull request as ready for review September 20, 2022 07:50
@thumbnail thumbnail requested review from a team, tcoenraad and neominik and removed request for a team September 20, 2022 07:50
@thumbnail thumbnail merged commit 6740b7c into main Sep 21, 2022
@thumbnail thumbnail deleted the spec-coerse.core branch September 21, 2022 11:35
@thumbnail thumbnail mentioned this pull request Nov 28, 2022
28 tasks
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