-
Notifications
You must be signed in to change notification settings - Fork 64
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
Consider using namespace qualified tagged literals #12
Comments
See also similar bug on aero: juxt/aero#44 |
Thanks for pointing this out. While edn says that tags should be prefixed, I have two reservations:
Remember also that: (ig/read-string "foo.edn") Is just a shortcut for: (edn/read-string {:readers {'ref ig/ref}} "foo.edn") And with Aero, you could write: (defmethod aero/reader 'integrant/ref [_ _ value]
(ig/ref value)) I think that in the rare circumstance that an un-namespaced |
It's also worth pointing out that Datomic "cheats" in this fashion as well. In transactions, |
I've not looked at the details, but will your proposal work, if you have a file containing both aero and integrant refs? You should ideally be able to use the same tags in the same file, it's the whole point of edn being extensible. Like integrant aero also defines a I'm all for bending the rules for brevity, aero plan to do it, and I'd certainly vote for an |
You can just give the integrant refs a different name. As I said in my previous comment: (defmethod aero/reader 'integrant/ref [_ _ value]
(ig/ref value)) So if you added that, you'd have (defmethod aero/reader 'ig/ref [_ _ value]
(ig/ref value)) You can't use |
I just watched the CodeNode video and had the same concern about the unqualified #ref. The potential for real-world conflicts is admittedly small within the scope of integrant config files, but it could be worse if integrant-style data was incorporated into some larger data format or processed by external tools. Probably at this point, you don't want to change your notation or invalidate existing config files. Perhaps, as a compromise, you could add an additional qualified tag, like #ig/ref, as a documented option doing the exact same thing. That would only take a small addition to ig/read-string. It would allow people who care to follow the Clojure rules to do so, and still be able to share config data without requiring extra user code. |
The issue with unqualified tags is that a future version of Clojure might define a meaning for the unqualified tag. |
I don't have any problem with breaking backward compatibility in pre-1.0 versions, as long as there's a good reason. I'll think about this a little more. However, I don't currently see there's any problem using |
I just got around to looking at integrant and came here to file this issue. :) In this particular case, of all the words you could pick, ref seems like it's near the top of the list as one of the most likely ones to conflict with potential core usage. What about #ig/ref? |
I'm leaning toward changing it to |
I appreciate the succinct nature of the namespace free reader tags e.g. #ref but ideally these tags would be namespaced #juxt.aero/ref or perhaps just #aero/ref. clojure.org has this to say:
And the edn-format spec re-echoes the same sentiment:
There may already be a collision here too, as aero also defines #ref. How would one use both aero and integrant together?
The text was updated successfully, but these errors were encountered: