cognitect-labs / aws-api

AWS, data driven
Apache License 2.0
724 stars 100 forks source link

.clj files in `examples` are scripts, not namespaces #221

Closed scottbale closed 1 year ago

scottbale commented 1 year ago

Dependencies

I'm working directly with a clone of this repo, git sha e47fe94049db196fc7f988f6bbfd7fbb0a20a508. I've also locally added

org.clojure/tools.namespace           {:mvn/version "1.3.0"}

to the :extra-deps of the :examples alias.

Description with failing test case

The .clj files located in the examples folder are all clojure scripts, but do not define namespaces (they have no ns forms).

The assume_role_example.clj script contains a qualified auto-resolved keyword.

I'm running a REPL using the examples alias, which adds the examples directory as an extra path. In the REPL, when I invoke tools.namespace refresh, the REPL prints an exception:

(clojure.tools.namespace.repl/refresh)
Execution error (ExceptionInfo) at clojure.tools.reader.impl.errors/throw-ex (errors.clj:34).
Invalid keyword: ::credentials/ttl.

With some debugging, I see that the read-file-ns-decl function is attempting to read a (nonexistent) ns form from each example script. But in attempting to read the assume_role_example.clj script, the reader runs afoul of the ::credentials/ttl keyword because credentials isn't (yet) a namespace alias.

Expected behavior

I'm not sure what I expect to happen, or what (if anything) should be done. I understand about the reader's behavior for qualified auto-resolving keywords, that it is peculiar but has behaved that way for a long time. But beyond that I have questions:

puredanger commented 1 year ago

This is perfectly fine Clojure code. Clojure's execution model is to read and evaluate one form at a time. clj files with namespaces are just code that happens to have a call to ns at the top. There are multiple ways a .clj file can be found or loaded - if loaded by namespace, then the namespace forms a path to the file, but there are several ways to load a file by path instead. So on your bullet questions, 1) yes, 3) no.

The crux here is why this is invalid and goes back I think to a weakness in how tools.namespace works, in that it uses ns forms to seed the autoresolve map. That is too weak to handle this (valid) code that does not declare a namespace to establish aliases.

If aws-api wanted to be friendlier to tools.namespace and other tooling in these examples, it could declare a namespace and the requires in that ns call.

dchelimsky commented 1 year ago

If aws-api wanted to be friendlier to tools.namespace and other tooling in these examples, it could declare a namespace and the requires in that ns call.

That introduces its own problem: these examples do things in whatever aws account the client can find credentials for, and we don't want to encourage "load". I've seen the .repl convention used elsewhere in order to convey that "this is a file with forms that you could evaluate one at a time at your discretion."

Your answer, @puredanger , to "Should the example scripts be renamed to have a different suffix?" was "no", but what about "do you think it would be reasonable to rename the example scripts with a different suffix?"

dchelimsky commented 1 year ago

Another convention that I've seen for example code with side effects is to wrap the side-effect-driving code in comment. What's your opinion about that?

puredanger commented 1 year ago

I think Clojure code should be in files with .clj (as that's what editors and tooling expect) and that drives syntax highlighting etc.

It would maybe be a good idea to put the side-effecting top-level statements in either functions and/or comment forms as applicable so that reload/refresh is safe.