Documentation for `module/expand-path` is wrong and confusing.

[amano-kenji]

Documentation for `module/expand-path` is wrong and confusing.

[sogaiu]

After looking at the current implementation, trying some invocations and revisiting the docstring, I think I've got a clearer sense of it.

To review part of the docstring:

    Expands a path template as found in `module/paths` for 
    `module/find`. This takes in a path (the argument to require) and a 
    template string, to expand the path to a path that can be used for 
    importing files.

I think an important point about this is the last bit of text:

to expand the path to a path that can be used for importing files

That is, it's a function with the purpose of producing paths that can be used for importing files, specifically in the context of use, import, and require.

Currently, use and import are basically wrappers around import*, and import* and require are wrappers around require-1. require-1 in turn calls module/find and that in turn is the sole direct user of module/expand-path in boot.janet:

              use    import
                \    /
                import*   require
                    \       /
                    require-1
                        |
                   module/find
                        |
                module/expand-path

So it seems to me that it's possible that module/expand-path is a rather special-purpose function that has a behavior tailored for use with module/find... and looking at the first sentence of the docstring:

Expands a path template as found in module/paths for module/find.

(^^;

---

As to the parts of the docstring that follow "The replacements are as follows", I also found some of them confusing (specifically :cur: and :dir:), but I'm still working on forming an understanding. The C code seems to be helping though.

[sogaiu]

TLDR:

:cur: seems more like a directory portion of (dyn :current-file) if there is a directory portion.

Regarding related commits:

• It looks like that's not what it used to be, and it changed to more or less the current behavior in this commit.

• The current tests (which demonstrate current behavior of :cur: and :dir:) seem to have been added on the same day (though earlier) in this commit.

• The portion of the docstring explaining the template bits seems to have been added some months later in this commit.

The first two commits mentioned above are from 2019-06, while the third one is from 2019-12.

One useful interpretation might be that the code is in its intended state and that the docstring is not aligned (as the issue claims).

Details below...

---

module/expand-path's implementation has this bit:

    /* Calculate dirpath from current file */
    const char *curname = curfile + strlen(curfile);
    while (curname > curfile) {
        if (is_path_sep(*curname)) break;
        curname--;
    }
    const char *curdir;
    int32_t curlen;
    if (curname == curfile) {
        /* Current file has one or zero path segments, so
         * we are in the . directory. */
        curdir = ".";
        curlen = 1;
    } else {
        /* Current file has 2 or more segments, so we
         * can cut off the last segment. */
        curdir = curfile;
        curlen = (int32_t)(curname - curfile);
    }

and this bit:

            } else if (strncmp(template + i, ":cur:", 5) == 0) {
                janet_buffer_push_bytes(out, (const uint8_t *)curdir, curlen);
                i += 4;

Also, there are these tests:

(setdyn :current-file "some-dir/some-file")
(defn test-expand [path temp]
  (string (module/expand-path path temp)))

(assert (= (test-expand "abc" ":cur:/:all:") "some-dir/abc")
        "module/expand-path 1")
(assert (= (test-expand "./abc" ":cur:/:all:") "some-dir/abc")
        "module/expand-path 2")
(assert (= (test-expand "abc/def.txt" ":cur:/:name:") "some-dir/def.txt")
        "module/expand-path 3")
(assert (= (test-expand "abc/def.txt" ":cur:/:dir:/sub/:name:")
           "some-dir/abc/sub/def.txt") "module/expand-path 4")

Based on the above information (and observed behavior) I agree that:

    * :cur: -- the current file, or (dyn :current-file)

seems off.

So to repeat, :cur: seems more like a directory portion of (dyn :current-file) if there is a directory portion.

---

On a side note, it seems that (dyn :current-file) is not necessarily going to be an absolute path, e.g.

$ mkdir sample
$ echo "(pp (dyn :current-file))" > sample/cf.janet
$ janet sample/cf.janet
"sample/cf.janet"

It looks like one place that the dynamic variable associated with :current-file is set is in run-context.

I think there are at least two ways it can be an absolute path though:

1. If one were to explicitly use setdyn to arrange for it
2. If janet is passed an absolute path

The second way can be demonstrated like:

$ mkdir /tmp/sample
$ echo "(pp (dyn :current-file))" > /tmp/sample/cf.janet
$ janet /tmp/sample/cf.janet 
"/tmp/sample/cf.janet"

---

Update: I don't have anything comparable about :dir: posted here, but I essentially agree with what amano-kenji mentioned above (based on a similar examination of the source, tests, commits, and exploration):

:dir: -- the directory containing the current file

It is the directory part of path argument.

[sogaiu]

Although I can see that one might think the explanation may not be plain enough on its own, I don't agree that:

Expands a path template as found in module/paths for module/find.

is "wrong and confusing" (what this issue is about).

Also, the docstring mentions module/paths and module/find, both of which have docstrings of their own. It seems reasonable to look at those as well to try to arrive at an understanding for module/expand-paths. Put differently, I think it's fair that not everything is in the docstring for module/expand-path.

[sogaiu]

I find the current text to no longer be "wrong" nor "confusing".

I think this issue has helped to bring about improvement in the existing documentation, but since #1373 was merged, I don't feel it needs more work [1].

If someone feels further changes might be helpful, they can submit a PR. If the content is deemed a net positive change, may be merging will follow :)

---

[1] (I'll add that I think docstrings can be helpful, but in Janet's case there is also website documentation as well that is complementary and I believe better for some things.)

[amano-kenji]

Okay. Closing for now.

[iacore]

The current path expansion logic feels unclean.

Janet doesn't have string interpolation, so this is more of a special case.

idea:

# current syntax
":cur:/:all:"

# new syntax   easier to understand
'(string (dyn :cur) "/" (dyn :all)) # pass to (eval)

[sogaiu]

Based on the initial report and some of the exploration above, I'm thinking of suggesting the following modification to the docstring:

diff --git a/src/core/corelib.c b/src/core/corelib.c
index c4dec6f0..e6a60d7b 100644
--- a/src/core/corelib.c
+++ b/src/core/corelib.c
@@ -116,8 +116,8 @@ JANET_CORE_FN(janet_core_expand_path,
               "* :@all: -- Same as :all:, but if `path` starts with the @ character, "
               "the first path segment is replaced with a dynamic binding "
               "`(dyn <first path segment as keyword>)`.\n\n"
-              "* :cur: -- the current file, or (dyn :current-file)\n\n"
-              "* :dir: -- the directory containing the current file\n\n"
+              "* :cur: -- the directory portion, if any, of (dyn :current-file)\n\n"
+              "* :dir: -- the directory portion, if any, of the path argument\n\n"
               "* :name: -- the name component of path, with extension if given\n\n"
               "* :native: -- the extension used to load natives, .so or .dll\n\n"
               "* :sys: -- the system path, or (dyn :syspath)") {

As a reminder, here are some of the relevant tests:

(setdyn :current-file "some-dir/some-file")
(defn test-expand [path temp]
  (string (module/expand-path path temp)))

(assert (= (test-expand "abc" ":cur:/:all:") "some-dir/abc")
        "module/expand-path 1")
(assert (= (test-expand "./abc" ":cur:/:all:") "some-dir/abc")
        "module/expand-path 2")
(assert (= (test-expand "abc/def.txt" ":cur:/:name:") "some-dir/def.txt")
        "module/expand-path 3")
(assert (= (test-expand "abc/def.txt" ":cur:/:dir:/sub/:name:")
           "some-dir/abc/sub/def.txt") "module/expand-path 4")

[sogaiu]

@amano-kenji I filed #1373 and it was merged.

Do you think it addressed your concerns? If so, please close this issue.

[amano-kenji]

🐈

[amano-kenji]

Expands a path template as found in module/paths for module/find.

Actually, this needs to be addressed, too. I don't think the explanation is plain enough. This can be a good summary of what it does for someone who already knows what it does. But, I'm still a newbie.

[amano-kenji]

Expands a path template as found in module/paths for module/find.

This is not wrong, but the language is confusing for newbies.

for module/find is a major source of confusion. for has various meanings in different contexts.... Don't try to explain everything in one sentence. That only leads to confusion. Break it up into multiple sentences.

I would write something like

Expands a path with a path template. The path template is described by (doc module/paths). module/find expands a path with path templates in module/paths. This function expands path argument with a path template specified by template argument.

If you read my j3blocks function doc, you would realize that I explain things in multiple sentences... I use simple unambiguous sentences. That's how newbies should learn new things...

In english, one word can have multiple meanings. I want to avoid connotations and confusions. I would try to explain something in several ways.

Most open-source developers are bad teachers... Explain it as you would to a newbie.

[pepe]

I am not a friend of the lengthy documentation in the code. And module/find is not the "newbie" functionality.

[amano-kenji]

Think of me as a newbie. That's not very long. That's only a little bit longer.

[pepe]

@sogaiu, I meant it the way you wrote it in [1] for the last comment. That is why I specifically wrote: "in the code."

And I do not see any newbie who needs to understand how modules are found and loaded. If they do, the language has failed them.