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

Consider using namespace qualified tagged literals #12

Closed
RickMoynihan opened this issue Mar 1, 2017 · 10 comments
Closed

Consider using namespace qualified tagged literals #12

RickMoynihan opened this issue Mar 1, 2017 · 10 comments

Comments

@RickMoynihan
Copy link
Contributor

RickMoynihan commented Mar 1, 2017

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:

Reader tags without namespace qualifiers are reserved for Clojure.

And the edn-format spec re-echoes the same sentiment:

Tag symbols without a prefix are reserved by edn for built-ins defined using the tag system.

There may already be a collision here too, as aero also defines #ref. How would one use both aero and integrant together?

@RickMoynihan
Copy link
Contributor Author

See also similar bug on aero: juxt/aero#44

@weavejester
Copy link
Owner

weavejester commented Mar 1, 2017

Thanks for pointing this out. While edn says that tags should be prefixed, I have two reservations:

  1. #integrant/ref is a quite a bit more to type than #ref
  2. We're not really using the config file to communicate with anything other than Integrant

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 #ref actually matters, you could instead use the edn/read-string function directly.

@weavejester
Copy link
Owner

It's also worth pointing out that Datomic "cheats" in this fashion as well. In transactions, #db/id is used as a tag, rather than #datomic.db/id or #datomic/id. The rules are bent slightly for the purposes of brevity.

@RickMoynihan
Copy link
Contributor Author

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 #ref, so how can you disambiguate an aero ref from an integrant one? Aero have agreed to prefix them with #aero/ref and support a deprecated legacy function for non prefixed ones (see juxt/aero#44 ), so the problem won't happen if they do that; but I think the burden is on all projects to namespace to prevent collisions such as this.

I'm all for bending the rules for brevity, aero plan to do it, and I'd certainly vote for an #ig/ref.

@weavejester
Copy link
Owner

weavejester commented Mar 1, 2017

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 #ref (or #aero/ref) for Aero refs, and #integrant/ref for Integrant refs. If you wanted brevity, you could also write:

(defmethod aero/reader 'ig/ref [_ _ value]
  (ig/ref value))

You can't use ig/read-string and aero/read-config together anyway, so there's no chance of it clashing by default, and since the configuration is a local resource, we don't expect anyone else to read it directly, either.

@miner
Copy link

miner commented Mar 20, 2017

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.

@miner
Copy link

miner commented Mar 20, 2017

The issue with unqualified tags is that a future version of Clojure might define a meaning for the unqualified tag.

@weavejester
Copy link
Owner

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 #ref as the default data reader for integrant.core/ref. And if using #ref does become an issue, then it's trivial to switch to a different symbol for the data reader.

@puredanger
Copy link

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?

@weavejester
Copy link
Owner

weavejester commented Apr 20, 2017

I'm leaning toward changing it to #ig/ref, but I'd like to think about it some more. In the meanwhile, it's trivial to change the data reader symbol to whatever you want; #ref is just the default.

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

No branches or pull requests

4 participants