-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RDY] stdpath() (#5297) #6272
[RDY] stdpath() (#5297) #6272
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly needs cosmetic changes. The function name is up for debate.
You might be interested in using nvimdev.nvim to help out with the development.
src/nvim/eval.c
Outdated
rettv->vval.v_string = NULL; | ||
|
||
if (STRCMP(p, "CONFIG") == 0) | ||
rettv->vval.v_string = (char_u *)get_xdg_home(kXDGConfigHome); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs braces. See: https://neovim.io/develop/style-guide.xml?showone=Conditionals#Conditionals
src/nvim/eval.c
Outdated
@@ -10399,6 +10399,24 @@ static dict_T *get_buffer_info(buf_T *buf) | |||
return dict; | |||
} | |||
|
|||
/* | |||
* "get_xdg_home(string)" function | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ///
for docstrings. See: https://neovim.io/develop/style-guide.xml?showone=Function_Comments#Function_Comments
runtime/doc/eval.txt
Outdated
@@ -2041,6 +2041,7 @@ garbagecollect([{atexit}]) none free memory, breaking cyclic references | |||
get({list}, {idx} [, {def}]) any get item {idx} from {list} or {def} | |||
get({dict}, {key} [, {def}]) any get item {key} from {dict} or {def} | |||
get({func}, {what}) any get property of funcref/partial {func} | |||
get_xdg_home({string}) String get the XDG Base directory for {string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be named something else as mentioned here: #5297 (comment)
I'm partial to config_path()
. There should also be less emphasis on XDG
since it wouldn't apply on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XDG does apply to Windows. See stdpaths.c
.
Though now that I think of it, it should probably be something closer to the stdpaths_user_conf_subpath()
and it's _data_
sibling since it outputs the nvim-data
directory for Windows.
I don't like config_path()
because there is a data and cache directory too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to the characters xdg
. On windows it's %APPDATA%
or %LOCALAPPDATA%
. XDG_DATA_HOME
could've been My Documents
on Windows. The relationship between XDG_*
and the Windows equivalents are purely a "best fit" decided by us. The point is, xdg
has no meaning on Windows.
I don't like config_path() because there is a data and cache directory too.
That's why I said it was up for debate :-)
My view is that it should be something that can be found using intuition. The keywords xdg
and home
don't say "the place where program configs and data is stored" to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with stdpath()
since it seemed the most correct.
runtime/doc/eval.txt
Outdated
@@ -3749,6 +3750,20 @@ get({func}, {what}) | |||
'dict' The dictionary | |||
'args' The list with arguments | |||
|
|||
get_xdg_home([{expr}]) *get_xdg_home()* | |||
Get the XDG Base directory for {expr}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Return" instead of "Get"
runtime/doc/eval.txt
Outdated
Get the XDG Base directory for {expr}. | ||
|
||
These are the directories used by Neovim loading and saving | ||
config, data and cache files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just "These directories are the default locations for various files used by Neovim." Then link to docs such as vimrc
and shada
.
runtime/doc/eval.txt
Outdated
component. | ||
|
||
{expr} can be one of the following: | ||
'CONFIG' The XDG_CONFIG_HOME directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also mention the Windows equivalents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys should be lowercase too. The env var names are irrelevant to this API: they aren't used at all if they are not defined. Only the semantic description is relevant, e.g.:
config user configuration directory, e.g. ~/.config
data user data directory, e.g. ...
cache cache data directory
|
Maybe wait for another review besides mine. @justinmk's suggestion for |
Also needs some tests. |
Okay, I renamed it to I didn't try getting it to return a List for If someone points me at similar code, I'd be happy to take a stab at it. |
Fixed the windows errors in the tests. D'oh! Also, I forgot to revert my experiment with |
Take a look at |
@tweekmonster No |
If I am not mistaking, it would be something like list_T *const list = tv_list_alloc();
rettv->v_type = VAR_LIST;
rettv->vval.v_list = list;
list->lv_refcount++;
void *iter = NULL;
do {
size_t dir_len;
const char *dir;
iter = vim_colon_env_iter(XDG_CONFIG_PATHS, iter, &dir, &dir_len);
if (dir != NULL && dir_len > 0) {
tv_list_append_string(list, dir, (ptrdiff_t)dir_len);
}
} while(iter != NULL); (for #5119, code would be a bit different for the master). |
Thanks for your help; Things I would appreciate help with:
Thanks again, guys! |
why not |
Because at the moment, they are |
@tweakmonster: Can you update your review? I think I made all the changes you requested. |
runtime/doc/eval.txt
Outdated
@@ -6995,6 +6996,30 @@ sqrt({expr}) *sqrt()* | |||
"nan" may be different, it depends on system libraries. | |||
|
|||
|
|||
stdpath([{what}]) *stdpath()* | |||
Returns the standard for for {what} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/for/path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also missing period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! I'll push up a fix in a moment.
describe('stdpath()', function() | ||
before_each(function() | ||
clear({env={ | ||
XDG_CONFIG_HOME='cat', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we test with more realistic values ? like /home/helmut/.config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is basically string manipulation; it doesn't know anything about separators. And I don't want to write tests that test get_xdg_home()
and friends; those tests belong someplace else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actually, edge cases are more likely to contain bugs. Though better test both realistic and not, I would also suggest using $XDG_DATA_HOME
and like here (i.e. check that environment variables are not expanded) and ~/foo
(i.e. check that ~/
is not expanded).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: All stdpath()
does is string manipulation. Testing that ~/foo
and $BAR
aren't expanded would be in the tests for stdpaths_get_xdg_var()
(see below).
If someone points me at the right direction, I can see about creating a test or 3 for stdpaths_get_xdg_var()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@docwhat stdpaths_get_xdg_var()
may only be tested in a unit tests. This is functional test, it tests how all that is functioning together. stdpaths_get_xdg_var()
may be splitted into multiple functions, replaced with some library function (e.g. if libuv will provide similar functionality), etc with tests gone, stdpath()
is going to stay, be a part of an API and questioning “how API works in edge cases” is completely valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to explain it to me. I have a commit with new tests that try to hit the edge cases specified here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
|
||
it('accepts unknown strings', function() | ||
eq('', eval('stdpath("cavey")')) | ||
end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need a check what new function will do if environment variable is changed at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that isn't a function of stdpath()
; stdpaths_get_xdg_var()
takes care of that for stdpath()
.
I don't see any tests for stdpaths_get_xdg_var()
but I may be missing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@docwhat It is a function of stdpath()
, stdpaths_get_xdg_var()
is not exposed directly to the user.
The point would be to use XDG_RUNTIME_DIR on platforms that support it, and sensible alternatives on other platforms. I would think windows has some per-user temp location that could be used. |
|
||
it('knows the data home', function() | ||
if os_name() == 'windows' then | ||
eq('dog\\nvim-data', eval('stdpath("data")')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest one line in all cases: eq(alter_slashes('capybara/nvim'), funcs.stdpath('cache'))
and eq(os_path('dog/nvim', 'dog\\nvim-data'), funcs.stdpath('data'))
: os_name()
checks may be hidden in two new pretty trivial functions alter_slashes
and os_path
. (funcs
is helpers.funcs
and it is better then eval
for this purpose.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions:
alter_slashes()
is a noop for Unix and converts/
to\
on Windows?os_path()
does what? Choses arg1 for Linux and arg2 for Windows?- Where would these go?
- Do I have to create
funcs.stdpath()
myself (and where)? Or is it created on its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@docwhat Yes; yes; at the start of the smallest scope possible; just import funcs
from helpers
, indexing will work automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to leave the os_name() == "windows"
in place for the case of appending /nvim-data
(windows) vs. /nvim
(everyone else) because I felt it was clearer. I did move it into a before_each
though to get it out of each test.
src/nvim/eval.c
Outdated
const char *dir; | ||
iter = vim_colon_env_iter(dirs, iter, &dir, &dir_len); | ||
if (dir != NULL && dir_len > 0) { | ||
char *dir_with_nvim = strncpy(xmallocz(dir_len), dir, dir_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use xmemdupz(dir,dir_len)
instead of strncpy(...)
.
src/nvim/eval.c
Outdated
xfree(dir_with_nvim); | ||
} | ||
} while (iter != NULL); | ||
xfree(dirs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code for config_dirs
and data_dirs
cases are nearly identical. Could be refactored.
runtime/doc/eval.txt
Outdated
need temporary files to work. Returns a | ||
String. | ||
'config_dirs' Additional configuration directories. | ||
This should treated as read-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... should be treated ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Not sure what I was thinking.
runtime/doc/eval.txt
Outdated
This should treated as read-only | ||
resources. Returns a List. | ||
'data_dirs' Additional data directories. | ||
This should treated as read-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... should be treated ...
?
runtime/doc/eval.txt
Outdated
@@ -6995,6 +6996,30 @@ sqrt({expr}) *sqrt()* | |||
"nan" may be different, it depends on system libraries. | |||
|
|||
|
|||
stdpath([{what}]) *stdpath()* | |||
Returns the standard path(s) for {what} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing dot.
runtime/doc/eval.txt
Outdated
@@ -6995,6 +6996,30 @@ sqrt({expr}) *sqrt()* | |||
"nan" may be different, it depends on system libraries. | |||
|
|||
|
|||
stdpath([{what}]) *stdpath()* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove []
, this argument is required.
src/nvim/eval.c
Outdated
const void *iter = NULL; | ||
|
||
if (strcmp(p, "config") == 0) { | ||
rettv->v_type = VAR_STRING; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this out of if()
, do not repeat: most of returns are strings anyway.
src/nvim/eval.c
Outdated
} | ||
} while (iter != NULL); | ||
xfree(dirs); | ||
} else if (strcmp(p, "data_dirs") == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is nearly identical to the previous branch, you can join them easily.
runtime/doc/eval.txt
Outdated
@@ -2254,6 +2254,7 @@ spellsuggest({word} [, {max} [, {capital}]]) | |||
split({expr} [, {pat} [, {keepempty}]]) | |||
List make |List| from {pat} separated {expr} | |||
sqrt({expr}) Float square root of {expr} | |||
stdpath({what}) String/List returns the standard path for {what} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other description below uses path(s)
instead of path
.
eq('chihuahua/nvim', gotten[1]) | ||
eq('terrier/nvim', gotten[2]) | ||
end | ||
eq(2, #gotten) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be eq({'chihuhua/nvim', 'terrier/nvim'}, gotten)
with new functions (not sure what would you want to choose: just use os_path
or special-case tables in alter_slashes
).
eq('siamese/nvim', gotten[1]) | ||
eq('persian/nvim', gotten[2]) | ||
end | ||
eq(2, #gotten) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
it('accepts unknown strings', function() | ||
eq('', eval('stdpath("cavey")')) | ||
end) | ||
end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test for errorring out on non-string (need [helpers.]exc_exec
or [helpers.]redir_exec
, redir_exec
is sometimes better because here you can test with return value on error: eq('…', redir_exec('echo string(stdpath([]))'))
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a number, a dictionary and a string and something is handling the dict and string case for me?
Vim(call):E731: using Dictionary as a String
Vim(call):E730: using List as a String
Are there other types that I'm not thinking aware of?
eq(2, #gotten) | ||
end) | ||
|
||
it('accepts unknown strings', function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see why it should accept them. Errorring out is better.
src/nvim/eval.c
Outdated
/// "stdpath(type)" function | ||
static void f_stdpath(typval_T *argvars, typval_T *rettv, FunPtr fptr) | ||
{ | ||
const char *p = tv_get_string(&argvars[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use tv_get_string_chk
and return early, so you would not give another error message for non-string when errorring out in else
.
src/nvim/eval.c
Outdated
xfree(dirs); | ||
} else { | ||
rettv->v_type = VAR_STRING; | ||
rettv->vval.v_string = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For almost any use-case it is better to error out: it makes bugs (e.g. typos, or code for more recent Neovim version) immediately visible at the place where they are and not in the place where unexpected empty string is being processed.
Fixes neovim#6664 until neovim#6272 is merged and sdtpath('data') can be used.
Fixes neovim#6664 until neovim#6272 is merged and sdtpath('data') can be used.
@tweekmonster: I think I have done everything you asked. Did I miss anything? |
@docwhat I apologize for the very late reply. |
runtime/doc/eval.txt
Outdated
|init.vim| is stored here. | ||
config_dirs List Additional configuration directories. | ||
data String User data directory. |shada-file| | ||
are stored here for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should read:
The |shada-file| is stored here.
like the init.vim
example above. If there is a reason to keep "for example", it needs a comma before it (otherwise it implies the reason the ShaDa file resides there is to serve as an example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I made that change.
@jamessan I think I got it fixed, now... |
Adds the :stdpath method for fetching XDG standard directories. Fixes #5297
I can't wait for this to be merged! |
FEATURES: 3cc7ebf #7234 built-in VimL expression parser 6a7c904 #4419 implement <Cmd> key to invoke command in any mode b836328 #7679 'startup: treat stdin as text instead of commands' 58b210e :digraphs : highlight with hl-SpecialKey #2690 7a13611 #8276 'startup: Let `-s -` read from stdin' 1e71978 events: VimSuspend, VimResume #8280 1e7d5e8 #6272 'stdpath()' f96d99a #8247 server: introduce --listen e8c39f7 #8226 insert-mode: interpret unmapped META as ESC 98e7112 msg: do not scroll entire screen (#8088) f72630b #8055 let negative 'writedelay' show all redraws 5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330 a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422) 207b7ca #6844 channels: support buffered output and bytes sockets/stdio API: f85cbea #7917 API: buffer updates 418abfc #6743 API: list information about all channels/jobs. 36b2e3f #8375 API: nvim_get_commands 273d2cd #8329 API: Make nvim_set_option() update `:verbose set …` 8d40b36 #8371 API: more reliable/descriptive VimL errors ebb1acb #8353 API: nvim_call_dict_function 9f994bb #8004 API: nvim_list_uis 3405704 #7520 API/UI: forward option updates to UIs 911b1e4 #7821 API: improve nvim_command_output WINDOWS OS: 9cefd83 #8084, #8516 build/win: support MSVC ee4e1fd win: Fix reading content from stdin (#8267) TUI: ffb8904 #8309 TUI: add support for mouse release events in urxvt 8d5a46e #8081 TUI: implement "standout" attribute 6071637 TUI: support TERM=konsole-256color 67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3) 3d0ee17 TUI/rxvt: enable focus-reporting d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior FIXES: ed6a113 #8273 'job-control: avoid kill-timer race' 4e02f1a #8107 'jobs: separate process-group' 451c48a terminal: flush vterm output buffer on pty output #8486 5d6732f :checkhealth fixes #8335 53f11dc #8218 'Fix errors reported by PVS' d05712f inccommand: pause :terminal redraws (#8307) 51af911 inccommand: do not execute trailing commands #8256 84359a4 terminal: resize to the max dimensions (#8249) d49c1dd #8228 Make vim_fgets() return the same values as in Vim 60e96a4 screen: winhl=Normal:Background should not override syntax (#8093) 0c59ac1 #5908 'shada: Also save numbered marks' ba87a2c cscope: ignore EINTR while reading the prompt (#8079) b1412dc #7971 ':terminal Enter/Leave should not increment jumplist' 3a5721e TUI: libtermkey: force CSI driver for mouse input #7948 6ff13d7 #7720 TUI: faster startup 1c6e956 #7862 TUI: fix resize-related segfaults a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output 303e1df #7624 TUI: disable BCE almost always 249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum 6f41ce0 #7704 macOS: Set $LANG based on the system locale a043899 #7633 'Retry fgets on EINTR' CHANGES: ad60927 #8304 default to 'nofsync' f3f1970 #8035 defaults: 'fillchars' a6052c7 #7984 defaults: sidescroll=1 b69fa86 #7888 defaults: enable cscopeverbose 7c4bb23 defaults: do :filetype stuff unless explicitly "off" 2aa308c #5658 'Apply :lmap in macros' 8ce6393 terminal: Leave 'relativenumber' alone (#8360) e46534b #4486 refactor: Remove maxmem, maxmemtot options 131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343 c57d315 #8031 jobwait(): return -2 on interrupt also with timeout 6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940 300d365 #7919 Make 'langnoremap' apply directly after a map ada1956 #7880 'lua/executor: Remove lightuserdata' INTERNAL: de0a954 #7806 internal statistics for list impl dee78a4 #7708 rewrite internal list impl
This adds a method to access the XDG Base Dirs home values.
A future patch could add the
get_xdg_dirs()
method to return lists.This will close issue #5297.
I apologize for my C code. The last C program I wrote was a game for the Palm Pilot.