Closed jeroenvandijk closed 1 year ago
I've marked it "ready for review" and I'm expecting some suggestions for improvement. Especially for the testing part I'm curious what you think.
Also the not sure what to do with the update of bbin
file
@jeroenvandijk: Thanks for the PR!
I've marked it "ready for review" and I'm expecting some suggestions for improvement. Especially for the testing part I'm curious what you think.
The change looks good overall. We have tests for bbin install *.jar
so that's enough for me. I try not to add any more tests than necessary to keep maintenance down to a minimum. That said, is there anything you think we should add?
Also the not sure what to do with the update of bbin file
Use the bb dev
task to start a watcher to update the bbin
file in the background. It's best to ensure this is in sync within each commit. I usually just leave bb dev
running until I'm done working on bbin
.
@rads Thank you for the feedback and simplification suggestion. I've applied the feedback.
I have also added an extra step to the install of a remote jar. After this change if the remote jar doesn't have a Main-Class the jar doesn't get cached in .babashka/bbin/jars
. E.g. you can try bbin install https://repo1.maven.org/maven2/org/clojure/clojure/1.11.1/clojure-1.11.1.jar
I couldn't run all the tests locally due to the dependency of a private repo:
{:args ("git" "clone" "--quiet" "--mirror" "git@github.com:rads/bbin-test-lib-private.git" "/Users/jeroen/.gitlibs/_repos/ssh/github.com/rads/bbin-test-lib-private"), :exit 128, :out "", :err "ERROR: Repository not found.\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.\n"}
at sci.lang.Var.invoke (lang.cljc:182)
sci.impl.analyzer$return_call$reify__4815.eval (analyzer.cljc:1351)
sci.impl.analyzer$analyze_throw$reify__4571.eval (analyzer.cljc:937)
sci.impl.analyzer$return_if$reify__4526.eval (analyzer.cljc:819)
Regarding the extra tests, maybe testing with a jar without a "Main-Class" and confirm bbin will fail? For http and local jars.
The existing tests are fixed now. I'm going to merge this in and do the final test/clean up as follow-up commits so I can finish this tonight.
Thanks @rads! I see you had to make some adjustments and even a fix for re-installing a jar. Should have caught that myself! Thanks again for bbin
.
All good, thanks for the contribution!
Resolves #46
Test with
bb gen-script && bb bbin install <your-jar>
I'm leaving this as a draft until I get to the following
bb ci
)Please answer the following questions and leave the below in as part of your PR.