Fork me on GitHub
#ring-swagger
<
2017-03-20
>
mrchance13:03:00

Is it really necessary to do it this way? It caused issues with our logging, if the logging lib wasn't required in the handler.clj namespace with the api

mrchance13:03:57

I have come to fear top level side effects, especially when coupled with eval Oo

mrchance13:03:49

More specifically: What problem does this solve?

bja14:03:50

@mrchance it lets you not use clojure.tools.logging if you don't want it

bja14:03:49

clojure.tools.logging also does some nasty stuff at the top-level w.r.t. figuring out which implementation to use (https://github.com/clojure/tools.logging/blob/master/src/main/clojure/clojure/tools/logging.clj#L288)

bja14:03:57

and by nasty stuff, I mean it just picks a logger implementation based on whatever random by trying to load a bunch of java classes in a loop with try/catch. if it finds one, then it evals a whole boatload of implementation stuff.

ikitommi15:03:35

@mrchance @bja yes, idea was not to require c.t.l as a dependency. If that is causing problems, let's fix them. Ideas welcome.

bja15:03:22

@ikitommi it doesn't bother me; I already had a namespace dependency order issue due to using timbre to control all of the java logging libraries (my timbre config namespace has to be loaded before any of my other code)

mrchance15:03:11

@bja That would bother me, but maybe that's just me 😉

mrchance15:03:18

We had the same issue, also using timbre

bja15:03:37

at the very least, it might be worth mentioning in documentation. something like "If you intend on debug messages and errors from this library using clojure.tools.logging, clojure.tools.logging needs to be loaded before requiring compojure-api namespaces."

mrchance15:03:58

We have a lot of relatively new Clojure developers, it's hard to explain something like that, and also making sure no one gets it wrong

ikitommi15:03:50

Idea in the code was to see if there is a c.t.l as a dependency, guess it doesn't do that now? Requires an manual import too?

ikitommi15:03:14

there is a test, but that could be broken too ;(

ikitommi15:03:27

also ring-swagger checks if there is Java1.8+ and adds swagger-bindings to java.time classes if there are.

ikitommi15:03:58

I agree that this should "just work".

bja15:03:48

user=> (compojure.api.impl.logging/log! :error "foo-bar")
Mar 20, 2017 10:29:31 AM clojure.tools.logging$eval420$fn__424 invoke
SEVERE: foo-bar

bja15:03:10

vs w/o clojure.tools.logging on the classpath:

user=> (compojure.api.impl.logging/log! :debug "foo-bar")
DEBUG foo-bar

bja15:03:25

(:debug is ignored by default by clojure.tools.logging)

bja15:03:57

that seems to work as-intended to me (use c.t.l. when available, default to prn otherwise)

bja15:03:49

@mrchance would you prefer a direct dependency on clojure.tools.logging?

mrchance15:03:35

That's what my coworker suggested, I am not sure how to best solve this

bja15:03:01

I think I've seen people use a multimethod with a default (for the console logging) and then a standalone namespace for implementations (shipped with c.t.l). The downside is that multimethod dispatch adds to the overhead and it would be a breaking change for people who use c.t.l. right now. Those people would suddenly need an extra require to continue working as-is.

bja15:03:12

@mrchance if you're unhappy with runtime eval at the top-level, how are you okay with c.t.l. to begin with?

mrchance15:03:14

I am not really in favor of c.t.l., I just want my own logging to behave deterministically, regardless of the order in which I require namespaces

bja15:03:39

@mrchance from my testing, the order of loading namespaces doesn't appear to matter to compojure-api

bja15:03:07

@mrchance if you create a new project and then add the latest compojure-api, you can require compojure.api.impl.logging and then use log! to see that the console prn implementation is selected. if you then add c.t.l. and restart your repl, you'll see that c.t.l. is used

mrchance15:03:38

I didn't check myself, but from what my coworker told me, it appears that errors are swallowed if the main compojure api using ns doesn't require c.t.l.

mrchance15:03:49

I asked him to get back to you with details

bja15:03:07

the only thing that would affect that would be if you muck around with your classloader at runtime (i.e. require some part of compojure-api and then add c.t.l. to your classloader and use it somewhere else. IMO that would be your own fault for mucking with the classloader)

mrchance15:03:42

I am reasonably sure we don't muck with the classloader at runtime 😉

juhoteperi15:03:58

Instead of find-ns, Compojure-api could check if (io/resource "clojure/tools/logging.clj") exists

juhoteperi15:03:35

find-ns only finds the namespace if it is already loaded, resource is in classpath if the c.t.l dependency is included

bja15:03:13

@juhoteperi that doesn't seem consistent with basic testing

bja15:03:36

i.e.

(defproject comp-api-ctl-test "0.1.0-SNAPSHOT"
  :description "FIXME: write description"
  :url ""
  :license {:name "Eclipse Public License"
            :url ""}
  :dependencies [[org.clojure/clojure "1.8.0"]
                 [metosin/compojure-api "1.2.0-alpha3"]
                 [org.clojure/tools.logging "0.3.1"]])

bja15:03:10

[[email protected] comp-api-ctl-test]$ lein repl
nREPL server started on port 40443 on host 127.0.0.1 - 
REPL-y 0.3.7, nREPL 0.2.12
Clojure 1.8.0
OpenJDK 64-Bit Server VM 1.8.0_121-b14
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> (require 'compojure.api.impl.logging)
nil
user=> (compojure.api.impl.logging/log! :error "foo-bar")
Mar 20, 2017 10:43:54 AM clojure.tools.logging$eval420$fn__424 invoke
SEVERE: foo-bar
nil

bja15:03:39

the find-ns clearly works without me loading clojure.tools.logging ahead of time

ikitommi15:03:32

require at macro-expansion time?

ikitommi15:03:05

(actually, at compile-time)

bja15:03:48

this still ends up happening at initial load of the namespace at runtime

bja15:03:59

the eval is syntax-quoted

bja15:03:42

although I like that more than find-ns or using io/resource, since clojure.core/require will actually verify that c.t.l. is available

juhoteperi15:03:55

Find-ns shouldn't see the namespace if it isn't already loaded

bja15:03:23

hmm, I wonder if the default lein repl profile somehow requires c.t.l....

juhoteperi15:03:54

user=> (find-ns 'clojure.tools.logging) nil

juhoteperi15:03:25

oh well in this case I don't even have the dependency, 1 sec

ikitommi15:03:35

(find-ns 'compojure.api.core)
nil

ikitommi15:03:29

so, need to call require to get it working as intended.

juhoteperi15:03:30

user=> (find-ns 'clojure.tools.logging) #namespace[clojure.tools.logging]

bja15:03:38

hmm, sure looks like lein repl is doing something that lies to me:

user=> (find-ns 'clojure.tools.logging)
#object[clojure.lang.Namespace 0x2ae8aefa "clojure.tools.logging"]
user=> (find-ns 'compojure.api.core)
nil

juhoteperi15:03:42

hmm somehow it loaded automatically

ikitommi15:03:02

might a used in a dependency far away...

juhoteperi15:03:20

shouldn't matter as this is empty repl

bja15:03:29

@ikitommi it's definitely safer to use that linked when-ns

bja15:03:46

since the require will actually verify that clojure.tools.logging gets loaded if it's available

ikitommi15:03:48

would someone like to do a PR?

bja15:03:26

is it safe to depend on our dependency's utility library?

ikitommi15:03:35

1.2.0 will use the one in Muuntaja for Manifold & Core-async btw.

bja15:03:37

or would you prefer that gets imported directly to compojure-api?

ikitommi15:03:48

I would import that.

ikitommi15:03:14

(will change the async to use that too)

juhoteperi15:03:11

what does this log! macro even do when the c.t.l is loaded

juhoteperi15:03:19

why does it log that "function name + invoke"

juhoteperi15:03:46

why is log! fn in else branch and macro in then branch

juhoteperi16:03:43

because c.t.l calls are macros