search

clj-commons / byte-streams

A Rosetta stone for JVM byte representations
417 stars 33 forks source link

Replace deprecated single-segment ns w Potemkin's import-vars #66

Closed KingMob closed 1 year ago

KingMob commented 1 year ago

Obviously, this introduces a dependency on Potemkin, but Aleph already needs it, and Potemkin has barely changed in years.

KingMob commented 1 year ago

Fixes #65

KingMob commented 1 year ago

@p-himik can you try this out?

KingMob commented 1 year ago

Fair point. I held off because I was debating whether to kill the top-level test namespaces entirely. Insofar as they test behavior, there's now no way the top-level tests can fail but the clj-commons tests will succeed. But they do still test that the fns are available and callable.

Even if I kill the other tests, I suppose I should keep that one around

On Thu, Dec 22, 2022 at 2:38 PM Arnaud Geiser @.***> wrote:

@.**** approved this pull request.

It's looking good. It means we'll have to release a new version of Aleph with this change. It was also causing us some issues, but as we were aware of the work that has been done on byte-streams, it was easy to do the migration on our side.

In src/byte_streams.clj https://github.com/clj-commons/byte-streams/pull/66#discussion_r1055134504 :

  • ;; compare the namespaces, make sure we got everything
  • (require '[clojure.set :as set])
  • (set/difference

Maybe this can be turned into a Clojure test and the whole comment block can be removed.

— Reply to this email directly, view it on GitHub https://github.com/clj-commons/byte-streams/pull/66#pullrequestreview-1227180612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHHB5NT7NBFCUQLUM7C3BTWOPZNLANCNFSM6AAAAAATGK7ERY . You are receiving this because you authored the thread.Message ID: @.***>