Fork me on GitHub
#polylith
<
2021-08-11
>
emccue04:08:04

I'm a bit confused on interfaces I think

emccue04:08:19

(ns dev.mccue.config.interface
  (:import
    (com.typesafe.config Config ConfigFactory)
    (java.util LinkedHashMap)
    ( Writer))
  (:refer-clojure :exclude [int boolean]))

(set! *warn-on-reflection* true)

;; ----------------------------------------------------------------------------
(defn load-from-classpath
  "Loads the default config from the classpath"
  []
  (ConfigFactory/invalidateCaches)
  (ConfigFactory/load))

;; ----------------------------------------------------------------------------
(defmethod print-method
  Config
  [^Config x ^Writer writer]
  (.write writer "#com.typesafe.config.Config")
  (.write writer (let [m (LinkedHashMap.)]
                   (doseq [[k v] (sort-by key (.entrySet x))]
                     (.put m k v))
                   (str m))))

;; ============================================================================
;; Helpers
;; ============================================================================

;; ----------------------------------------------------------------------------
(defn- opt-string
  "Helper for optionally extracting a string from the config"
  [^Config config key]
  (when (.hasPath config key)
    (.getString config key)))

;; ----------------------------------------------------------------------------
(defn- int
  "Helper for extracting an int from the config"
  [^Config config key]
  (.getInt config key))

;; ----------------------------------------------------------------------------
(defn- boolean
  "Helper for extracting a boolean from the config"
  [^Config config key]
  (.getBoolean config key))

;; ----------------------------------------------------------------------------
(defn- string
  "Helper for extracting a string from the config"
  [^Config config key]
  (.getString config key))

;; ============================================================================
;; Config Values
;; ============================================================================

;; ----------------------------------------------------------------------------
(defn server-port
  "The port on which to listen for http requests."
  [config]
  (int config "server.port"))

;; ----------------------------------------------------------------------------
(defn db-pool-connections?
  "Whether or not to pool connections to the database."
  [config]
  (boolean config "db.pool_connections"))

;; ----------------------------------------------------------------------------
(defn db-log-queries?
  "Whether or not to pool connections to the database."
  [config]
  (boolean config "db.log_queries"))

;; ----------------------------------------------------------------------------
(defn db-url
  "The name of the database to use for primary storage"
  [config]
  (string config "db.url"))

;; ----------------------------------------------------------------------------
(defn db-username
  "The username for the database"
  [config]
  (opt-string config "db.username"))

;; ----------------------------------------------------------------------------
(defn db-password
  "The password for the database"
  [config]
  (opt-string config "db.password"))

;; ----------------------------------------------------------------------------
(defn redis-url
  "The url for connecting to the redis cluster."
  [config]
  (string config "redis.url"))

;; ----------------------------------------------------------------------------
(defn production?
  "Whether the code is running in a \"production\" environment"
  [config]
  (= (or (opt-string config "env") "production")
     "production"))

;; ----------------------------------------------------------------------------
(defn non-production?
  "Whether the code is running in a \"non-production\" environment"
  [^Config config]
  (not (production? config)))

;; ----------------------------------------------------------------------------
(defn photos-s3-bucket
  "The s3 bucket to store photos into. nil if undefined."
  [config]
  (opt-string config "photos_s3_bucket"))

;; ----------------------------------------------------------------------------
(defn support-email
  "The s3 bucket to store photos into. nil if undefined."
  [config]
  (string config "support_email"))

;; ----------------------------------------------------------------------------
(defn jwt-secret
  "The secret key to use when signing a JWT"
  [config]
  (string config "jwt_secret"))

;; ----------------------------------------------------------------------------
(defn stripe-secret
  "The secret key to use when signing a JWT"
  [config]
  (string config "stripe_secret"))

emccue04:08:22

this is an example namespace from my toy project - it has a defmethod which exists as a side effect, a "constructor", and "instance methods"

emccue04:08:45

is the recommendation to move all the impl here into another namespace and (def server-port impl/server-port)

emccue04:08:43

i'm always fine biting repetition/verbosity for a reason, but I don't see the "contract" benefit of the interface being an explicit thing

seancorfield04:08:55

@emccue Yeah, that's something that seems really "off" about Polylith when you first start -- so much apparent boilerplate and repetition. In a namespace like this, it seems particularly hard to justify since all your functions are one-liners and maybe I'd leave all the public one-liners in place but move the private stuff (and the defmethod which is purely a side-effecting def) into impl.clj and reference it like that.

seancorfield04:08:23

Over time, if any functions become more complex, move their implementation to impl.clj and delegate to them.

seancorfield04:08:16

But I have to say, that seems like a very ad hoc set of functions to all have in one namespace and it seems to defy Clojure's data-first principles...

seancorfield04:08:29

I think I would be inclined to reify the whole config at load time into a Clojure data structure, including spotting "true"/`"false"` and any parsable integeres.

seancorfield04:08:43

(since you can get .entrySet of Config, that seems easy to do -- does the config auto-reload behind the scenes or otherwise mutate from time to time?)

emccue04:08:57

I did that in one draft - including verification and whatnot config object style

seancorfield04:08:04

We've gotten used to the delegation because it provides a nice commonality of approach, and it means that interface.clj can always be in alphabetical order if you want, regardless of any impl.clj interdependencies. Also, some of our components are very small, e.g.,

;; copyright (c) 2012-2021 world singles networks llc

(ns ws.file-system.interface
  (:require [ws.file-system.impl :as impl]))

(set! *warn-on-reflection* true)

(defn directory-list
  "Given a directory and a regex, return a sorted seq of matching filenames."
  [dir re]
  (impl/directory-list dir re))
and
;; copyright (c) 2012-2021 world singles networks llc

(ns ws.file-system.impl
  (:require [ :as io]))

(set! *warn-on-reflection* true)

(defn- wildcard-filter
  "Given a regex, return a FilenameFilter that matches."
  [re]
  (reify java.io.FilenameFilter
    (accept [_ dir name] (not (nil? (re-find re name))))))

(defn directory-list
  "Given a directory and a regex, return a sorted seq of matching filenames."
  [dir re]
  (sort (.list (io/file dir) (wildcard-filter re))))

emccue04:08:43

but i wouldn't take this that seriously - its just my toy project to try out libraries and other stuff

seancorfield04:08:49

(over time I expect that to accrue other functions as we refactor -- but we've being very careful to split code into very specific named components)

seancorfield04:08:47

We have quite a few components that just have a single function in their interface.clj

seancorfield04:08:57

Focusing on naming and modularity like that can also lead you to design a narrower interface (which is why I'd be tempted, in your case, not to have a function for every property but to make it more generic somehow, even if it's just reifying ILookup over the underlying Config object -- which would lead to a nice, narrow interface and everything else hidden away in impl.

imre07:08:04

There are 2 things I don't really get wrt interface namespaces. 1 - why my.component.interface and my.component.impl as opposed to my.component and my.component.impl? 2 - if the interface ns has a require .impl at the top, there is a direct source code dependency from the interface to the implementation, making consumers of the interface transitively but still at compile time depend on the impl. Doesn't that defeat the purpose of an interface somewhat? And if it isn't a goal of these interfaces to serve as means of dependency inversion, I'll double down on my 1st point.

imre10:08:10

But I've yet to fully read through the documentation so the answer to these could be in there

seancorfield15:08:22

@U08BJGV6E This is probably the hardest part of Polylith to get used to. The naming is "just" a convention but the consistency pays off in several areas. The swappability of the components comes in assembly as each project within the workspace can declare which components it depends on -- and two different components can implement the same interface, so when a project is assembled, the specific component implementing the shared interface can be selected. There's an example in the docs with user and user-remote having the same interface and different implementations.

seancorfield15:08:07

I advocated for my.component vs my.component.interface but there's a lot of existing Polylith code and supporting both formats would be a lot of extra complexity in the tooling and would require a lot of additional testing and documentation. I've talked with @U1G0HH87L several times about this and, now that we're well into our migration journey, we've gotten used to .interface and it actually helps us keep track of which code is coming from a refactored Polylith component and which is still legacy. Seeing my.component.interface in a :require clause is a nice visual clue (and we alias away the .interface part anyway).

imre15:08:10

Thanks for that, Sean! Do you by any chance remember where those user/remote components are a little more precisely? I can't seem to find them so far

seancorfield15:08:28

The consistency comes in always having components/<name>/src/<top-ns>/<name>/ being a folder, with all code inside it -- and the tooling also uses the .interface on the ns as a "flag" when checking dependencies between various pieces of code, so it can easily tell you if code violates the "interface" by depending on non-interface code instead.

seancorfield15:08:04

I think the user/`user-remote` example is in the poly tool documentation (but it might be in the high-level docs?). Let me find it for you...

imre15:08:54

Thank you!

seancorfield15:08:00

You get used to the .interface convention pretty quickly 🙂

tengstrand15:08:05

You have the final state of the example application https://github.com/polyfy/polylith/tree/master/examples/doc-example also.

imre15:08:36

ah, that's actually a very helpful link

imre15:08:48

Hmmm.... now there's another itch I have: se.example.user.core meaning two different things, with the interface ns duplicated.

imre15:08:18

I suppose I have to spend some more time coding through the examples to fully understand the benefits

seancorfield15:08:11

Yeah, you won't really "get it" until you've used it for a while, in my experience.

seancorfield15:08:13

Example from the file I'm working in right now:

(ns api.handlers.membership
  (:require [clojure.tools.logging :as logger]
            [ring.util.response :as resp]
            [worldsingles.i18n :as i18n]
            [worldsingles.libraries.oauth2 :as oauth2]
            [worldsingles.membership :as membership]
            [worldsingles.money :as money]
            [ :as site]
            [worldsingles.user :as u]
            [ws.billing-sdk.interface :as billing]
            [ws.configuration.interface.protocols :as cfg-p]
            [ws.web.request :as req]))
spot the two migrated components compared to the legacy code 🙂

seancorfield15:08:13

Since our legacy subprojects tended to be large and a bit amorphous, figuring out where those other namespaces are is non-trivial unless you work with them a lot.

imre15:08:48

Yeah the migrated ones stand out. But what do you mean by "figuring out where those other namespaces are is non-trivial"?

seancorfield16:08:33

@U08BJGV6E In our legacy codebase, our subprojects are much coarser-grained than components so there's no obvious clue in the namespace name alone as to which subproject the code is from. Without the discipline/focus of something like Polylith, our codebase has grown somewhat ad hoc over the last decade.

imre16:08:05

ah, I see, thank you, that makes total sense

seancorfield16:08:14

In that snippet shown above, the worldsingles and ws nses that are not obviously components (with .interface) come from three different subprojects.

imre16:08:19

We also have something like that in our codebase

imre16:08:05

A handrolled monorepo of utility libraries where namespace structure is not really well maintained

seancorfield16:08:36

When we started using Clojure, we thought we'd have a fairly large, flat project so everything was worldsingles.*. That got hard to manage so we switched new code to ws.<concept>.something but <concept> did not necessarily mean subproject -- we were just adding another level to provide grouping/organization. With Polylith, as soon as we see ws.<something>.interface, we *know* it's in components/<something>, guaranteed!

Karol Wójcik10:08:43

What does s & t means?

Matt Ielusic21:08:24

While migrating legacy code, I have discovered - the hard way - that poly test won't run tests in files outside the top-level namespace; is this intentional?

seancorfield22:08:54

Are you saying you have code inside components or bases that doesn't follow the src/<brick-name>/<top-ns>/<stuff>/.clj pattern?

Matt Ielusic22:08:03

Yeah...

components
|- src
  |- top-level
    |- brick
  |- legacy.namespace
|- test
  |- top-level
    |- brick.test
  |- legacy.namespace.test
And the tests in legacy.namespace.test is being ignored by poly test.

seancorfield22:08:42

I'm not very surprised it ignores them -- but it also would never have occurred to me to migrate code/tests in without renaming the namespaces... 🙂

seancorfield22:08:15

The "worst" part of our migration to Polylith is all the namespace renaming 😐

Matt Ielusic22:08:54

Of course, now I have a number of tests which I thought were passing, but which were in fact silently failing - or rather, being silently ignored - and probably have been for some time. And I am somewhat upset by this state of affairs.

seancorfield02:08:25

@U0295UQ75FG Out of curiosity, does clojure -M:poly check (or poly check) complain about those legacy tests being there?

seancorfield02:08:19

(I added a "bogus" test to a component and ran check and it complained about it)

tengstrand03:08:51

Hi. I guess your legacy code is imported as :local/root @U0295UQ75FG? Yes, the test runner only runs migrated tests (living in bricks). We could add support for running tests in code imported with :local/root if that’s the best way to go. Then I think we should also force people to specify which :local/root tests we should run. Some of them could be “plain libraries” also, and we should not run tests for these (that’s how I think tools.deps https://github.com/cognitect-labs/test-runner treats all :local/root imports, by never including their tests). We could show a warning if any of them are not specified. It’s never good if people think a tool behaves in one way, but turn out to behave in another way, so you have a valid point here, and it could also improve the migration experience. I don’t know if some people would start abusing this by adding code this way instead of putting it in bricks, in greenfield workspaces, but I hope not.

seancorfield04:08:26

I don't think poly can be expected to run non-brick tests that are elsewhere in the workspace -- there's no way to know how the deps are built for those tests, nor how to actually run those tests.

seancorfield04:08:36

One thing I do miss right now with poly test is how to run some setup per test run (not per namespace nor per test). For Cognitect's test-runner we have our own wrapper that handles per-test-run fixtures -- it would be nice to have a hook in poly test per-project to have an extra fixture.

seancorfield04:08:39

@U1G0HH87L Matt showed his component structure above with some legacy tests inside the component -- that just don't match the top-ns.brick.* naming convention: it's not imported test code.

seancorfield04:08:26

@U1G0HH87L It's not about :local/root imported code.

tengstrand04:08:59

Yes, I realised that too late!

tengstrand04:08:35

We have a warning (as you also wrote) that is triggered if you put code that does not live under the top namespace within a brick, and I think that should be enough.

seancorfield04:08:33

I don't see the warning with poly test tho', only with poly check

seancorfield04:08:49

Here's a repro:

[email protected]:/Developer/workspace/wsmain/clojure$ clojure -M:poly check
  Warning 205: Non top namespace legacy was found in preview-web.
[email protected]:/Developer/workspace/wsmain/clojure$ clojure -M:poly test project:preview
Projects to run tests from: preview

Running tests from the preview project, including 2 bricks: seo-json-renderer, preview-web
2021-08-11 21:40:15.748:INFO::main: Logging initialized @16102ms to org.eclipse.jetty.util.log.StdErrLog

Testing ws.preview-web.main-test

Ran 0 tests containing 0 assertions.
0 failures, 0 errors.

Test results: 0 passes, 0 failures, 0 errors.

Testing ws.seo-json-renderer.interface-test

Ran 2 tests containing 6 assertions.
0 failures, 0 errors.

Test results: 6 passes, 0 failures, 0 errors.

Testing ws.seo-json-renderer.impl-test

Ran 0 tests containing 0 assertions.
0 failures, 0 errors.

Test results: 0 passes, 0 failures, 0 errors.
Execution time: 3 seconds
[email protected]:/Developer/workspace/wsmain/clojure$ cat bases/preview-web/test/legacy/old_test.clj
(ns legacy.old-test
  (:require [clojure.test :refer [deftest is]]))

(deftest bad-test
  (is (= 1 2) "Broken!"))

seancorfield04:08:48

poly check correctly warns that legacy is non-conforming but poly test just quietly ignores it. So a failing test "doesn't fail" and you would only know if you ran check.

tengstrand04:08:48

Today the test command will refuse to execute if there are any errors, but it doesn’t show warnings. One idea could be to show a warning at the end of the test run, that these tests have been ignored.

tengstrand04:08:33

Or always show all warnings, that would be even simpler.

seancorfield04:08:14

Ah, OK. Yeah, that would be a good idea I think (depending on what other warnings it would otherwise suppress 🙂 )

tengstrand04:08:26

I will include that the next time I have something to push.

Matt Ielusic20:08:06

A bit late but... poly check was warning about the namespace problem... but it wasn't an error so I didn't think it was a huge problem. Whoops, I guess. And I've been working on updating the namespaces, but my codebase uses a lot of namespaced keys and I'm wary of moving them.

tengstrand20:08:17

No problem. The warning can maybe be explicit about that the tests are ignored + to show warnings at the end of the test commands (if any). I can put that on my todo list.

seancorfield20:08:21

@U0295UQ75FG I know this probably doesn't help (and you might not enjoy hearing it), but this is why it's better to have qualified keys that map to the business domain, instead of implementation details (actual code namespaces). :: style keywords should really be private to the code that uses them, except for keywords that you expand via aliases (and when you rename namespaces, you would have to update the :require/`:as` anyway so :: uses will continue to work unchanged).

seancorfield20:08:38

We use qualified keys a lot, but most of ours don't match actual code namespaces -- except for Specs which are typically private to a namespace (or accessed solely via ::alias/key names elsewhere).

Matt Ielusic21:08:36

My code uses Integrant, and Integrant depends on multimethods with ns-qualified keywords, like:

(defmethod integrant.core/init-key ::something [_ config] (...))
If the key isn't :: style then Integrant has trouble finding the correct multimethod. And then that key has to go in .edn files to get configured. And then I guess somebody eventually writes something like
(when (:legacy.namespace.file/something some-map) (...))
And then I move legacy.namespace.file and a test breaks.

seancorfield21:08:44

makes notes... adds this to the list of reasons why he wouldn't use Integrant... Sorry @U0295UQ75FG that does sound pretty painful 😞

🤷 2
😀 3
imre13:08:48

afaik integrant's keyword->namespace finding algo is extensible so it might be a matter of tweaking that

seancorfield22:08:23

(similarly for test/<brick-name>/...)

seancorfield04:08:36

One thing I do miss right now with poly test is how to run some setup per test run (not per namespace nor per test). For Cognitect's test-runner we have our own wrapper that handles per-test-run fixtures -- it would be nice to have a hook in poly test per-project to have an extra fixture.