fix behavior of nth with default argument

[phtrivier]

Closes #275

@frenchy64:

• I tried adding a unit test, and I added some other test cases. I don't know if there is already a "compatibility" test for all this core clojure stuff ?

Note that tests where not all passing on my machine, so I had to run it as /bin/test -tc=nth

• I don't know if the naming is proper for the test

• I don't know if there is a formater, etc...

[frenchy64]

Regarding the unit tests, they're in a separate repo that aspires to be a common base of tests for all Clojure distributions. Tests for core functions won't be accepted here.

Instead, add your tests here https://github.com/jank-lang/clojure-test-suite/blob/main/test/clojure/core_test/nth.cljc and make sure they at least work in clj/cljs.

Then remove them from this PR.

We're in an unfortunate period where the unit tests for nth don't yet work in jank, as documented here.

So please manually try them with jank repl and paste the results in the PR.

In terms of the change, it looks good to me. Could you make the diff a single line, just the return statement without ws changes or tests?

There is a formatter, but it shouldn't bother this PR. I use git clang-format main to fix up my formatting.

[frenchy64]

Could you also add Closes #275 in the PR description so merging this PR closes the issue?

[phtrivier]

Added tests in nth.cljc would look like this:

;; `nth` accepts a default argument
  (is (= :default (nth nil 0 :default)))
  (is (= :default (nth [0] 1 :default)))
  (is (= :default (nth [0 1] 2 :default)))

They pass when running lein test in jank/compiler+runtime/test/bash/clojure-test-suite/clojure-test-suite

Ran 124 tests containing 2949 assertions.
0 failures, 0 errors.

I've added them in the subrepo in another PR : jank-lang/clojure-test-suite#82

[frenchy64]

Thanks!

[jeaye]

Thanks for the quick change! Please address the CI failure. It failed because the branch you changed is now identical to the branch above it, so they can be merged.

After making the change, please request another review.

[jeaye]

Thanks for the ping. I don't see any changes, though. 🙃 Not going to trigger CI again, since it'll say the same thing.

Right now the code has this.

    if(index < 0)
    {
      return fallback;
    }
    else if(o == obj::nil::nil_const())
    {
      return fallback;
    }

Static analysis is complaining about these two branch bodies being identical, so we need to combine them.

    if(index < 0 || o == obj::nil::nil_const())
    {
      return fallback;
    }

[phtrivier]

Oh, sorry, I misread your comment and I thought you were talking about git branches, not the conditional branches 🤦

I'll fix that, sorry again!

[phtrivier]

I fixed the code, normally.

[jeaye]

Thanks for the fix!