Skip to content

Things like java-time[.api]/truncate-to do not work correctly when underlying protocol is extended #95

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

Closed
camsaul opened this issue Dec 24, 2022 · 5 comments

Comments

@camsaul
Copy link

camsaul commented Dec 24, 2022

In Metabase we extended the Truncatable protocol with some tweaks that we needed to handle certain situations:

https://github.com/metabase/metabase/blob/d0be9fc4c291438204e792f3981bf7fe5a42878a/src/metabase/util/date_2.clj#L289-L298

Previously, we were using a fork of 0.3.3, where the java-time namespace used Potemkin import-vars, which meant that java-time/truncate-to would be updated when the underlying java-time.core/truncate-to var changed (when using extend/extend-protocol/etc.

The newer version of this namespace just uses def to capture the value of java-time.core/truncate-to when the namespace is loaded; any subsequent changes to the underlying var no longer cause java-time/truncate-to or java-time.api/truncate-to to be updated.

I spent a little while trying to debug

java.lang.IllegalArgumentException: No implementation of method: :truncate-to of protocol: #'java-time.core/Truncatable found for class: java.time.LocalDate

errors in Metabase after upgrading to the latest version of this library until I figured out what was going on.

Switching back to Potemkin would prevent others from running in to issues like these. Alternatively, instead of something like

(ns java-time.api ...)

(def truncate-to java-time.core/truncate-to)

maybe you could do

(def truncate-to #'java-time.core/truncate-to)

or even

(defn truncate-to [t unit] (java-time.core/truncate-to t unit))

so the var gets resolved each time you use it.

@frenchy64
Copy link
Collaborator

frenchy64 commented Jan 4, 2023

Thanks for the detailed report. I'll think it over.

Notes:

@frenchy64
Copy link
Collaborator

@camsaul tangentially related, do you compile your app with direct linking?

frenchy64 added a commit that referenced this issue Jan 5, 2023
frenchy64 added a commit that referenced this issue Jan 5, 2023
@frenchy64
Copy link
Collaborator

Fixed in 1.2.0.

@camsaul
Copy link
Author

camsaul commented Jan 11, 2023

@frenchy64, no we don't use direct linking

@frenchy64
Copy link
Collaborator

@camsaul thanks. I since learnt that protocols methods are never directly linked.

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

2 participants