Clarify :fresh usage in import

[sarna]

This confused me a lot, because I kept calling (import ./mymodule :fresh) and it wouldn't reload anything.

[sogaiu]

That does seem clearer.

I guess if it had errored when using :fresh with no argument, it might have been less confusing as well?

[sarna]

Yup! Especially if there was "did you mean ..." in the error message. I'll try to write a guard like that.

[sarna]

@sogaiu Done. I tried fitting the style of asserts already in place.

These error out properly:

(import ./mymod :fresh)
(import ./mymod :as mine :fresh)

This one also works like it should (doesn't error out):

(import ./mymod :fresh nil)

This one still passes the wrong args further down, but I don't think there's much we can do about that:

(import ./mymod :fresh :as mine)

[sogaiu]

May be there's some way to add tests for the cases you posted?

FWIW, I see some testing of import in suite-boot.janet, though I didn't look too closely if the testing would be relevant here 😅

---

My impression is that import's current design is such that there is an expectation of an even number of things appearing after (import <something> and the "names" (i.e. :as, :exit, :export, :fresh, :only) are all keywords.

So for the last case:

(import ./mymod :fresh :as mine)

if we check that ps has an even number of elements and the first element of each pair of ps is a keyword, would that be helpful?

It might not check for :fresh specifically, but at least the last case might get flagged as problematic?

[sarna]

Thank you @sogaiu, based on your comment I managed to make it better :) Please check if the current code is okay.

[sogaiu]

Thanks for your continued efforts :)

---

Nice to see some tests -- I think it could be worth also adding the last two examples you mentioned in this comment. WDYT?

---

Sorry I was mistaken in my description in this comment earlier about:

ps has an even number of elements

I think it's not the length of ps that we want to be even but rather the length of args.

In any case, for:

  (def ps (partition 2 args))
  (each p ps
    (def k (first p))
    (assert (= (length p) 2) (string/format "%j needs a value" k))
    (assert (keyword? (first p)) (string/format "expected keyword, got %s: %j" (type k) k)))
  (def argm (mapcat (fn [[k v]] [k (case k :as (string v) :only ~(quote ,v) v)]) ps))

how about something like the following?

  (assertf (even? (length args)) "args should have even length: %n" args)
  (def ps (partition 2 args))
  (def argm
    (mapcat (fn [[k v]]
              (assertf (keyword? k) "expected keyword, got %s: %n" (type k) k)
              [k (case k :as (string v) :only ~(quote ,v) v)])
            ps))

This way we only traverse ps once and if args doesn't have even length [1] we bail without reaching the call to partition.

---

[1] The docstring for length says:

Returns the length or count of a data structure in constant time as
an integer. For structs and tables, returns the number of key-value
pairs in the data structure.

I think it's cheap to determine the length (it's a lookup IIUC). Although for import the length of args is not likely to be long so it may not matter much.

[sarna]

I added your suggestions and cleaned up a bit. Let me know what you think

[sogaiu]

LGTM.

I hope others are ok with it too :)