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

Fixed spec.cljc NS declaration to require clojure.string #109

Closed
wants to merge 2 commits into from

Conversation

Quezion
Copy link
Contributor

@Quezion Quezion commented Sep 11, 2019

Eliminates warning from some build-tooling on use of undeclared var.

Specifically, I was getting this warning on every build/reload my shadow-cljs browser app during development.

------ WARNING #1 - :undeclared-var --------------------------------------------
 Resource: zprint/spec.cljc:381:14
 Use of undeclared Var clojure.string/ends-with?
--------------------------------------------------------------------------------

Please edit as desired & thanks for the library!

Eliminates warning from some build-tooling on use of undeclared var
@kkinnear kkinnear self-assigned this Oct 3, 2019
@kkinnear kkinnear added the bug label Oct 3, 2019
@kkinnear
Copy link
Owner

kkinnear commented Oct 3, 2019

Thanks for this, I will fix this in the upcoming release!

@kkinnear
Copy link
Owner

I'm integrating this into the upcoming major release, and I am unable to reproduce this issue with my figwheel setup. Of course, I'm using the older lein-figwheel, not figwheel-main, so maybe that is the difference.

Anyway, in looking more carefully at your proposed fix, I can't see any reason why the (require clojure.string ...) shouldn't be outside of the reader conditional. That is to say, it seems to me like both Clojure and Clojurescript should have the require for clojure.string. The code that I'm going with at this point looks like this:

(ns ^:no-doc zprint.spec
  #?@(:cljs [[:require-macros [zprint.smacros :refer [only-keys]]]])
  (:require clojure.string
            #?@(:clj [[zprint.smacros :refer [only-keys]]
                      [clojure.spec.alpha :as s]]
                :cljs [[cljs.spec.alpha :as s]])))

and l left the (clojure.string/ends-with? ...) call alone.

If you have the inclination to test this and see if it solves the problem that you encountered, that would be a huge help -- since I can't reproduce it. I am going to try to release zprint toward the end of next week (i.e., maybe Oct 16 to Oct 18), so it would be best if you gave me feedback before then, but any feedback any time is always welcome. I have plenty to do before I get the release out, as this will be a big one and there is a good chance the release will slip beyond Oct 18.

Thanks again!

@Quezion
Copy link
Contributor Author

Quezion commented Oct 12, 2019

@kkinnear I tried your changes on local they seem fine -- the warning is gone. Updated PR with the tested code -- looks good & appreciate the follow-through.

The tool I was getting the error from was ttheller's shadow-cljs on version 2.8.45, originally forked off the [re-frame template.[(https://github.com/Day8/re-frame-template) Looks like the compiler is more sensitive than other CLJS build tooling.

@kkinnear
Copy link
Owner

I fixed this in 0.5.0, unfortunately not by using your pull request because of the massive development I had backed up. If you would like to issue a pull request to, say, the readme with a small change I'd be glad to accept it so that you get noted as a contributor.

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

Successfully merging this pull request may close these issues.

2 participants