search

clj-commons / manifold

A compatibility layer for event-driven abstractions
1.01k stars 106 forks source link

Upgrade to core.async 1.6.673 #231

Closed tanzoniteblack closed 1 year ago

tanzoniteblack commented 1 year ago

Some of the functions that we were relying on to hijack the go state machine compilation changed namespaces. This updates the code to reference where they now live.

Addresses https://github.com/clj-commons/manifold/issues/229

tanzoniteblack commented 1 year ago

Sorry for the slow response here, @KingMob . I've been thinking this over and I'm not sure that the complications of implementation make it worth supporting both. I think having a minimum version pinned as our provided scope, and warning in the release that if you upgrade to this version there's a particular minimum version you're expected to have of core.async. At least, my interpretation of provided dependencies has always been, "We tested with this particular version of the library. Caveat emptor if you choose to use a different version."

Let me know your thoughts?

KingMob commented 1 year ago

Hmmm. I guess my problem with "Caveat emptor if you choose to use a different version" is that usually means "We haven't tested with your version, and we can't predict what will happen". But in this case, we know it will break, and we know how to fix it. It feels like we're punting a problem to the user. OTOH, a quick Github search doesn't show anyone using it, so... ¯\_(ツ)_/¯

I think, to be safe, I'd still rather support both namespaces.

tanzoniteblack commented 1 year ago

@KingMob , I've validated locally that the change I made works with both the current & older version of core.async, but I'm unsure the best way to set up circleci to actually test both

KingMob commented 1 year ago

Great work!

To test both, maybe add two extra profiles in project.clj? One :old-core-async and one :new-core-async with the right deps. Then run both like:

lein with-profiles +old-core-async test :only whatever.the.go-off-tests.namespace.is
lein with-profiles +new-core-async test :only whatever.the.go-off-tests.namespace.is

I think that would work

tanzoniteblack commented 1 year ago

@KingMob , I added a new profile explicitly for the old core.async version and have the default deps list using the newer version, rather than adding profiles for both. Circleci config has also been updated to run the go-off tests. I've verified locally that the different profiles are correctly using the right core.async versions.

KingMob commented 1 year ago

Fixes #229