Two general comments if you don't mind, as you tagged me in the original issue:

1. I like that you used Caffeine, but I'd bring it in without Cloffeine (opinionated)
2. Noisy diffs due to auto formatting, even in cases where I agree with them, I would still avoid them as they make life harder for reviewers. For example, I agree with ns macro formatting, but not with binding alignment. Tastes vary.

More specifically, if you bring in caffeine and a protocol, I'd split them up in two namespaces and make Caffeine a managed dependency, such that pulling in Porsas won't pull it in by default. The implementation will be in another namespace which will be required by requiring-resolve on instantiating a caffeine cache. It will also make load times faster.

In the caffeine implementation namespace you can then defrecord like you normally would.

I'd also be interested in seeing an async loading cache, especially combined with the async vertx client.

Two general comments if you don't mind, as you tagged me in the original issue:

Sure thing, thanks for the review! :)

1. I like that you used Caffeine, but I'd bring it in without Cloffeine (opinionated)

As I could see, it's a very lightweight wrapper, not sure it's that big of an overhead. Although it's only used in one ns and not much of a code so I'm fine using just Caffeine.

2. Noisy diffs due to auto formatting, even in cases where I agree with them, I would still avoid them as they make life harder for reviewers. For example, I agree with ns macro formatting, but not with binding alignment. Tastes vary.

I used clojure-lsps default formatting which is kind of the standard, that's why I dared to leave the formatting changes in. One could use github's hide whitespaces feature to hide these changes. I could ofc revert these changes if they are that problematic.

More specifically, if you bring in caffeine and a protocol, I'd split them up in two namespaces and make Caffeine a managed dependency, such that pulling in Porsas won't pull it in by default. The implementation will be in another namespace which will be required by requiring-resolve on instantiating a caffeine cache. It will also make load times faster.

In the caffeine implementation namespace you can then defrecord like you normally would.

This is a good idea, originally I was thinking just to leave one of the implementations in, so that would be used by default.

I'd also be interested in seeing an async loading cache, especially combined with the async vertx client.

Could you elaborate on this pls? Not sure that can be used here, to calculate the cached value we need to have the RowSet for the given sql but probably it wouldn't make sense to execute the given sql against the database in order to calculate the row and cache it. Maybe I'm missing something here.

As I could see, it's a very lightweight wrapper, not sure it's that big of an overhead. Although it's only used in one ns and not much of a code so I'm fine using just Caffeine.

And if tomorrow there's a bug in Caffeine and you need to wait for Cloffeine's maintainers to update a transitive dependency?
Note there's also incompatibility with java 8 in later Caffeine versions.

Using Caffeine directly means less dependencies, less opinions, and less moving pieces.

I used clojure-lsps default formatting which is kind of the standard, that's why I dared to leave the formatting changes in.

You might be surprised if you run a survey across all Clojure programmers. I work with about 200 of them who don't use it.

One could use github's hide whitespaces feature to hide these changes.

Git is not Github, tomorrow we might all end up using another platform. When it comes right down to it, even good formatting changes, combined with logical changes, are noise. Some of them are not just whitespace changes. There's also an underlying assumption that patch readers and reviewers work only via Github's interface. Another thing to remember is that diffs are for the future, too. Let's assume there's a bug introduced by a commit. What properties should that commit have to make it easier to review?

A preliminary formatting PR is perfectly legit if you intend to change formatting, but I can only share my experience in that some developers would reject such patches almost on principle.

I could ofc revert these changes if they are that problematic.

In the end, I'm not the code owner, these are mostly conclusions I had to learn the hard way.

Could you elaborate on this pls? Not sure that can be used here, to calculate the cached value we need to have the RowSet for the given sql but probably it wouldn't make sense to execute the given sql against the database in order to calculate the row and cache it. Maybe I'm missing something here.

Thinking of returning a CompletionStage via loading cache and completing the computation onComplete on it.

And if tomorrow there's a bug in Caffeine and you need to wait for Cloffeine's maintainers to update a transitive dependency? Note there's also incompatibility with java 8 in later Caffeine versions.

Using Caffeine directly means less dependencies, less opinions, and less moving pieces.

Totally valid points, let me try to rework that part.

You might be surprised if you run a survey across all Clojure programmers. I work with about 200 of them who don't use it.

You are right, I'm surprised 😄

Git is not Github, tomorrow we might all end up using another platform. When it comes right down to it, even good formatting changes, combined with logical changes, are noise. Some of them are not just whitespace changes. There's also an underlying assumption that patch readers and reviewers work only via Github's interface. Another thing to remember is that diffs are for the future, too. Let's assume there's a bug introduced by a commit. What properties should that commit have to make it easier to review?

A preliminary formatting PR is perfectly legit if you intend to change formatting, but I can only share my experience in that some developers would reject such patches almost on principle.

Thanks for sharing your experience, I'm quite new to contributing to open source, I'll consider these in the future.

Thinking of returning a CompletionStage via loading cache and completing the computation onComplete on it.

hmm, let me try that, I don't have lots of experience withCaffeine.

@bsless @ikitommi I cleaned up the PR a little during the weekend and also separated the caching implementations into their own ns as @bsless suggested.
I'll look into how the async feature of caffeine could be used with CompletableFuture and have a separate PR about it as with the current protocol-based implementation it's not trivial (it would be probably easier if only caffeine-based caching would be supported)