Closed amano-kenji closed 8 months ago
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.
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:
setdyn
to arrange for itjanet
is passed an absolute pathThe 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.
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)
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")
@amano-kenji I filed #1373 and it was merged.
Do you think it addressed your concerns? If so, please close this issue.
:cat2:
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.
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
.
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 bytemplate
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.
I am not a friend of the lengthy documentation in the code. And module/find
is not the "newbie" functionality.
Think of me as a newbie. That's not very long. That's only a little bit longer.
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.)
@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.
Okay. Closing for now.
This was really confusing without reverse-engineering
module/expand-path
by calling it many times with different arguments. If this was an IQ test, I would fail.It is the directory part of
path
argument.This is the directory part of the janet file that's compiled. It's not the directory of the current source file. Let's assume that
test.janet
is at $HOME.test.janet's content
If the current working directory is $HOME.
If the current working directory is
/
.:cur:
is really not what the documentation says it is.