search

babashka / pods

Pods support for JVM and babashka
Eclipse Public License 1.0
120 stars 12 forks source link

Feature: declarative pods #44

Closed cap10morgan closed 2 years ago

cap10morgan commented 2 years ago

OK confirmed that 1b77a936d69c342f1cf6afcb1c3bbc9e6e001a45 fixes the image size increase issue (and fixes a bug too)

Fixed replacement for #43

borkdude commented 2 years ago

@cap10morgan Do you see any opportunities to add a test for the thing that got fixed, so it won't break again in the future?

cap10morgan commented 2 years ago

@cap10morgan Do you see any opportunities to add a test for the thing that got fixed, so it won't break again in the future?

Hmm... not sure what that would look like as a general purpose thing. Unless all usage of clojure.core/resolve is invalid in babashka? Then maybe we could test for that somehow.

Assuming it is sometimes valid, the fix I put in place was to stop shadowing the core.resolve fn name in the extracted fn. Might make sense to do that in the calling fn too (i.e. stop destructuring the keyword and just pull it out of the map where we use it). Let me know if you want me to do that.

borkdude commented 2 years ago

@cap10morgan What I meant was:

and fixes a bug too

Did we see that bug failing due some failing test? If not, it might be good to add such a test?

cap10morgan commented 2 years ago

@cap10morgan What I meant was:

and fixes a bug too

Did we see that bug failing due some failing test? If not, it might be good to add such a test?

Oh that bug! :)

No, I didn't. You'd have to add a test that simulated an EDN pod with a data reader.

I can look into it.