Fix #292: babashka compatibility

[borkdude]

Hi @weavejester - this fixes and tests babashka compatibility.

I took the liberty to add a generated deps.edn based off the project.clj. This makes testing this library as a babashka dependency easier but also benefits deps.edn users as this library can be used as a git dep. To re-generate the deps.edn, all you have to do is run bb gen-deps.

Also added: a Github CI config file for testing this library with babashka. I did not know how to work with Travis.
cc @lread

[borkdude]

Note that the test runner requires a recent version of babashka (as babashka only recently became compatible with eftest).

[pesterhazy]

Very cool. Quick benchmark:

numbers_of_files runtime_in_seconds

bb -m cjfmt.main check

1 0.174
2 0.207
4 0.286
8 0.460
16 0.761
32 1.392
64 2.636
128 5.113

clojure -m cjfmt.main check

1 1.629
2 1.680
4 1.779
8 1.890
16 2.066
32 2.421
64 3.109
128 4.473

Due to startup overhead, clojure takes 9.3x time compared to bb for 1 file. At 128 files, clojure overtakes bb and now takes 0.8x of bb's time.

Conclusion: bb is much faster if you're only checking a small number of files (e.g. on save in an editor or in a pre-commit git hook)

[weavejester]

Thanks for the PR. I'd rather not manage two different CI solutions, so either we convert everything over to GitHub Actions (or Circle CI) in another PR, or we instead use Travis. Equally, I think we'd want to split out deps.edn into another PR if its absolutely necessary (again, I'd rather maintain one dependency file than two).

[weavejester]

Can you explain why this is a macro, instead of something like:

(def bb? (boolean (System/getProperty "babashka.version")))

[borkdude]

We can do that if you prefer, but imo it's cleaner to not have any side effects of macro-expansions in conditionals happening at all, which can only be prevented by using another macro. But in this case it's probably ok.

[weavejester]

Does Babashka have a reader conditional in that case? i.e. would #?(:bb ...) work?

[borkdude]

@weavejester I solved this using a more elegant solution: I added a .bb file (which is always loaded first in bb) with the same namespace and vars so the main namespace doesn't need any changes. A reader conditional would have worked, but then we'd needed to rename the .clj namespace to .cljc.

[weavejester]

My current inclination would be to migrate from Travis to GitHub Actions, I think.

[borkdude]

I can remove the deps.edn and Github actions in this PR. In that case there won't be any verification (beyond manually running it) that the added bb support actually works. I'll have to rely on lein classpath to set the classpath for babashka.

I don't have time (nor desire) to learn Travis.

[weavejester]

Keep the GitHub Actions for now; I'll use it for reference as part of another PR. I've been meaning to switch a project over to GitHub Actions for evaluation, and this seems like a good opportunity to do so.

[borkdude]

@weavejester The only thing left is if you want to keep this deps.edn:

https://github.com/weavejester/cljfmt/pull/293/files#diff-f65a787e85051fde7ed000568b4cfb9277627049a38f0569c8d9e238e70b622b

I can generate it dynamically in CI to test bb or use lein classpath but if you want to support deps.edn at some point anyway, I think it would be good to leave it in as it simplifies testing with babashka. I would understand if you'd rather leave it out, so please let me know.

[borkdude]

This change was needed since bb loads this namespace during tests and cljs.test isn't part of bb. Loading clojure.test also works in ClojureScript so this change doesn't change any behavior.

[borkdude]

If you don't like the formatting of the generated deps.edn, of course we can do something about that now cljfmt works in bb ;).

[borkdude]

Note that adding a deps.edn has also been requested in #133

[borkdude]

(again, I'd rather maintain one dependency file than two).

The solution I've chosen uses lein2deps which automatically generates deps.edn file from a project.clj so you'd only have to maintain the project.clj one (and call bb gen-deps) until you decide to migrate.

[borkdude]

To reduce friction even more, I just published a lein plugin for lein2deps:

https://github.com/borkdude/lein2deps#lein-plugin

[weavejester]

The solution I've chosen uses lein2deps which automatically generates deps.edn file from a project.clj so you'd only have to maintain the project.clj one (and call bb gen-deps) until you decide to migrate.

There was also a project that I recall did the opposite: given a deps.edn file, it would fill in the dependencies in Leiningen. The advantage of doing it that way around is that there's no need to put generated files in the repository.

Ultimately I'd like to move over to tools.deps, but Leiningen handles deploying to Clojars or other Maven repositories better than any tool I've discovered for tools.deps. I've been attempting to fix this, but it hasn't been particularly interesting work, and I've kept putting it off.

[borkdude]

I’d be happy to add a build.cllj file but this would be even a bigger change. I can get to a deployable project with build.clj in seconds since I shifted that uninteresting work to “neil add build”. Just let me know how to proceed and I’ll do it, or if you want, I’ll create another PR first to address this other issue, happy to help.

[weavejester]

The tools.deps ecosystem isn't mature enough for me to switch, yet, so don't worry about it. Leiningen currently has better pom generation and GPG integration, last I checked, so I'm waiting for tools.deps to get up to par (or eventually, I'll fix it myself).

[borkdude]

Sure. Can you let me know how to finish up this PR?

[weavejester]

Sure. Can you let me know how to finish up this PR?

Ah, sorry for the confusion, I should have been much clearer about my intent. I was planning on converting the repository over to GitHub Actions, then getting back to you. That's now done.

Could you rebase against master and instead of the bb.yml workflow, could you add the Babashka tests to the new test.yml workflow? My thought is something like:

      - name: Install clojure tools
        uses: DeLaGuardo/setup-clojure@10.1
        with:
          lein: 2.9.10
          bb: latest

      - name: Run Leiningen tests
        working-directory: ./cljfmt
        run: lein test

      - name: Run Babashka tests
        working-directory: ./cljfmt
        run: bb test

My understanding is that Babashka requires deps.edn, unless it pulls the classpath from Leiningen. Is that correct? If so, let's keep deps.edn, as anything else feels like a hack. However, could you also remove gen-deps from this pull request, as I'd like to keep it focused on the minimal changes necessary to implement this feature.

Also, for now, I think it might make more sense for the test:bb job to be test, at least until there's different test profiles to run.

Finally, could you squash all the commits and change the final commit message to:

Add support for Babashka (fixes #292)

This is just to make it more consistent with previous commits.

[borkdude]

Absolutely, will do :)

[borkdude]

@weavejester Should be all good now.

[borkdude]

It seems I forgot to add bb in the github runner, will do it now.

[borkdude]

Done. If you can re-trigger CI once more, that would be great.

[weavejester]

Looks good!

[borkdude]

Thanks!