[vim/vim] Collation (#6229)
Christian Brabandt
allow to use collation based sorting of strings.
Currently, we use strcmp, which does a case sensitive comparison (almost) everywhere (we also use strcasecmp). For collation based sorting, we need to switch using strcoll(). I don't know whether this is available everywhere, so add a configure check and in case it is not available, fall back to strcmp. Apparently, on windows, we at least use strcoll() for generating the prototypes, so it should work there already.
For now, only apply the collation based sorting only for the readdirex() vimscript function, as asked for in #6214. I guess if this is useful and works well, we can move to strcoll() function for other functions as well.
Also, since it is now possible to use strcoll(), make it possible to query and set the collation based locale, using :lang collate as well as :echo v:collate.
Add a couple of tests to verify the behaviour.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/6229
Commit Summary
- Use strcoll for sorting string according to the locale
- Make collation order available in Vim
- Expand :lang command correctly
- Tests for readdirex
- Add some more completions for :lang command
File Changes
- M runtime/doc/eval.txt (11)
- M runtime/doc/mlang.txt (11)
- M src/auto/configure (2)
- M src/cmdexpand.c (3)
- M src/config.h.in (1)
- M src/configure.ac (2)
- M src/evalvars.c (1)
- M src/ex_cmds2.c (22)
- M src/fileio.c (2)
- M src/testdir/test_cmdline.vim (15)
- M src/testdir/test_functions.vim (29)
- M src/vim.h (10)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Bram Moolenaar
I can see that collation-ordered sorting can be useful, but for the readdir() function I thought the question was for case-ignored sorting. So README, Readme and readme sort together. And I would expect at least some users wanting byte-order sorting (like we do nearly everywhere currently).
Christian Brabandt
So README, Readme and readme sort together.
Yes, that's what I believe :lang collate de_DE would cause. If you do not want this, use :lang collate C
Bram Moolenaar
Christian wrote:
> > So README, Readme and readme sort together.
>
> Yes, that's what I believe `:lang collate de_DE` would cause. If you
It's generally not a good idea to have the result of a function depend
on a global variable. It tends to break plugins.
A generic, but possibly complicated solution, is to add an argument to
specify the sorting:
none - byte order
icase - current case-folding comparison
collate - using language from environment
collate:{lang} - collattion using specific language
If we introduce collation here, then no doubt will it be requested where
we currently have a choice between ignore-case and byte order.
--
** Hello and Welcome to the Psychiatric Hotline **
If you are obsessive-compulsive, please press 1 repeatedly.
If you are co-dependent, please ask someone to press 2.
If you have multiple personalities, please press 3, 4, 5 and 6.
If you are paranoid-delusional, we know who you are and what you want
- just stay on the line so we can trace the call.
If you are schizophrenic, listen carefully and a little voice will
tell you which number to press next.
If you are manic-depressive, it doesn't matter which number you press
- no one will answer.
If you suffer from panic attacks, push every button you can find.
If you are sane, please hold on - we have the rest of humanity on the
other line and they desparately want to ask you a few questions.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Christian Brabandt
Okay, will rework.
K.Takata
A generic, but possibly complicated solution, is to add an argument to specify the sorting:
none - byte order
icase - current case-folding comparison
collate - using language from environment
collate:{lang} - collattion using specific language
How about:
none - unsorted
case - byte order (default)
icase - current case-folding comparison
...
Returning the result without sorting may have speed advantage.
chdiza
A generic, but possibly complicated solution, is to add an argument to specify the sorting
Yes, this corresponds with my original request and seems like the least likely to cause problems.
I was about to alter my feature request after realizing that I was asking for a set of conditions that are jointly unsatisfiable. I wanted the results of readdirex() to (i) match those of Vimscript glob(), and (ii) match what one would get by sorting using Vimscript '>?'
I also realized that the latter is probably what I really want for my plugin.
Thus an optional flag to sort in that fashion is most welcome! (I don't know whether that is what is meant by "icase - current case-folding comparison").
Gary Johnson
> A generic, but possibly complicated solution, is to add an argument to
> specify the sorting:
> none - byte order
> icase - current case-folding comparison
> collate - using language from environment
> collate:{lang} - collattion using specific language
>
> How about:
>
> none - unsorted
> case - byte order (default)
> icase - current case-folding comparison
a stationary sort. So, if locale shows LC_COLLATE="en_US.UTF-8",
and the OS happened to put README, Readme and readme in the
directory in this order, for example,
Readme
readme
README
Vim's "sort i" would leave them in that order, but Vim's glob("*")
would sort them as
README
Readme
readme
and both ":!sort" and ":!echo *" would sort them as
readme
Readme
README
I still don't think that a purely case-insensitive sort is what
should be used for readdirex() or readdir() because the results will
be in an order dependent upon the order in which the OS happened to
put them in the directory. Two directories containing the same
files could case-insensitively sort differently.
Making a case-insensitive file name comparison can be useful, but
for sorting a directory, we should pick a set of collations or
unsorted. Call one dictionary order or case-folded, but not
case-insensitive.
Regards,
Gary
vim-dev ML
chdiza
Whatever the sorting order for readdirex() is, it should be possible to use Vimscript to return to that sorting order after first sorting by (e.g., filesize or time). That's why I have landed on "the same as >?".
So unless the category expr5 has comparison operators that will follow "collation", I do not want "collation order" as the only alternative to the current "case sensitive" ordering.
Sketch of example: let foo be the result of calling readdirex(). It's a list of dicts. It has some sorting order to it, where what's sorted "on" are the filename entries in each dict. I might want to use Vimscript to sort foo by the "time" entry. And then having inspected that, I might want to use Vimscript return to sorting foo by filename. This last step ought to put foo back in the order it was in when it was first created by readdirex(). Or rather, the optional arg for readdirex() that I'm requesting should be able to take a value that allows for this putting-back.
IIUC collation order will not allow this, because IIUC >? and friends don't follow collation order.
Now one might say "well, just sort foo by >? right after it's created but before you display any of its contents." But that puts me right back where I was when I opened the GH issue: the time needed to perform that initial sorting cancels out the time saved by using readdirex() instead of glob() in the first place.
I used to think that Vimscript's glob() put things in my desired order (on macOS at least), but I now think I was wrong about that.
—
You are receiving this because you commented.
Gary Johnson
> Whatever the sorting order for readdirex() is, it should be possible to use
> Vimscript to return to that sorting order after first sorting by (e.g.,
> filesize or time). That's why I have landed on "the same as >?".
>
> So unless the category expr5 has comparison operators that will follow
> "collation", I do not want "collation order" as the only alternative to the
> current "case sensitive" ordering.
>
> Sketch of example: let foo be the result of calling readdirex(). It's a list of
> dicts. It has some sorting order to it, where what's sorted "on" are the
> filename entries in each dict. I might want to use Vimscript to sort foo by the
> "time" entry. And then having inspected that, I might want to use Vimscript
> return to sorting foo by filename. This last step ought to put foo back in the
> order it was in when it was first created by readdirex(). Or rather, the
> optional arg for readdirex() that I'm requesting should be able to take a value
> that allows for this putting-back.
list with repeatable, predictable results.
Here's an example. I have a directory that contains three files.
If I read the directory unsorted at all, using either "ls -U" or
"find", I get this list in this order.
readme
README
Readme
If I apply Vim's case-insensitive sort, ":sort i", to that list,
I get the same list, because Vim's :sort is stationary.
readme
README
Readme
Now let me get the files sorted by time by reading the directory
using "ls -t".
README
readme
Readme
If I apply Vim's case-insensitive sort to that list I get the
following.
README
readme
Readme
Note that the list order is unchanged, as expected. Note also that
this order is different from what it was the first time I applied
Vim's case-insensitive sort to the "raw" file list. There is no
way to get back to Vim's case-insensitive sort of the original "raw"
list order using any sorting method other than one specifically
crafted to do that.
If you want all the same letters to sort together, and have
repeatable results so that you can put a list back in that order,
you need to choose a collating order, putting either each of the
lower-case letters before its upper-case equivalent or vice versa.
That is, either
aAbBcC...qQrRsS...
or
AaBbCc...QqRrSs...
> IIUC collation order will not allow this, because IIUC >? and friends don't
> follow collation order.
> Now one might say "well, just sort foo by >? right after it's created but
> before you display any of its contents." But that puts me right back where I
> was when I opened the GH issue: the time needed to perform that initial sorting
> cancels out the time saved by using readdirex() instead of glob() in the first
> place.
C code of the readdirex() function.
> I used to think that Vimscript's glob() put things in my desired order (on
> macOS at least), but I now think I was wrong about that.
yet. I thought it used the shell, but it doesn't.
Regards,
Gary
vim-dev ML
Christian Brabandt
reworked.
—
You are receiving this because you commented.
chdiza
@chdiza commented on this pull request.
> @@ -7989,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
+ case sort case senstive
+ icase sort case insensitive
+ collate sort using collation order of your
+ |locale|
+ Other values are silently ignored.
+
+ For example, to get a list of alle files in the current
Typo: "all", not "alle". Also, one line below: "entries", not "entires".
—
You are receiving this because you commented.
Christian Brabandt
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -7989,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
+ case sort case senstive
+ icase sort case insensitive
+ collate sort using collation order of your
+ |locale|
+ Other values are silently ignored.
+
+ For example, to get a list of alle files in the current
thanks fixed.
—
You are receiving this because you commented.
Christian Brabandt
Hm, looks like there are some sorting failures on some CI builds. Not sure how to fix those. I especially find the S390 results strange.
—
You are receiving this because you commented.
Gary Johnson
> reworked.
Looks good. That should keep everybody happy.
I noticed a couple of typos.
runtime/doc/mlang.txt:49:
collation order is pringed. Technical: LC_COLLATE.
^^^^^^^
"pringed" should be "printed".
src/testdir/test_functions.vim:1946:
" Skipt tests on Mac OS X (does not allow several files with different caseing)
^^^^^ ^^^^^^^
"Skipt" should be "Skip".
"caseing" should be "casing".
Also, in the new :help entry for readdirex(), it is not clear what
is meant by "case sensitive" here (which is also misspelled
"senstive"):
sort How to sort the result returned from the system.
icase sort case insensitive
byte value of each character", or 'using the collation order of the
"POSIX" or "C" locales'. (See the strcoll(3) man page.)
Regards,
Gary
vim-dev ML
K.Takata
@k-takata commented on this pull request.
> @@ -7931,7 +7939,7 @@ readdir({directory} [, {expr}]) *readdir()*
Can also be used as a |method|: >
GetDirName()->readdir()
<
-readdirex({directory} [, {expr}]) *readdirex()*
+readdirex({directory} [, {expr} [, {dict}]]) *readdirex()*
The same change is needed at the line 2691 (in |functions|).
In src/fileio.c:
> @@ -4638,10 +4642,16 @@ create_readdirex_item(char_u *path, char_u *name)
compare_readdirex_item(const void *p1, const void *p2)
{
char_u *name1, *name2;
+
Is this empty line needed?
In src/fileio.c:
> if (withattr) qsort((void*)gap->ga_data, (size_t)gap->ga_len, sizeof(dict_T*), compare_readdirex_item); else # endif sort_strings((char_u **)gap->ga_data, gap->ga_len); +
Is this empty line needed?
> @@ -7980,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
+ case sort case senstive
⬇️ Suggested change
- case sort case senstive + case sort case sensitive (default)
In src/fileio.c:
> @@ -4872,7 +4887,7 @@ delete_recursive(char_u *name) exp = vim_strsave(name); if (exp == NULL) return -1; - if (readdir_core(&ga, exp, FALSE, NULL, NULL) == OK) + if (readdir_core(&ga, exp, FALSE, NULL, NULL, 0) == OK)⬇️ Suggested change
- if (readdir_core(&ga, exp, FALSE, NULL, NULL, 0) == OK) + if (readdir_core(&ga, exp, FALSE, NULL, NULL, READDIR_SORT_NONE) == OK)
Actually, this changes the current behavior, but that may not be an issue.
In src/filepath.c:
> @@ -1424,7 +1424,7 @@ f_readdir(typval_T *argvars, typval_T *rettv)
expr = &argvars[1];
ret = readdir_core(&ga, path, FALSE, (void *)expr,
- (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem);
+ (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem, 0);
This changes the current behavior. Should be:
⬇️ Suggested change- (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem, 0); + (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem, READDIR_SORT_BYTE);
In src/filepath.c:
> @@ -1469,6 +1469,32 @@ readdirex_checkitem(void *context, void *item)
return retval;
}
+ static int
+readdirex_dict_arg(typval_T *tv, int *cmp)
+{
+ char_u *compare;
+
+ if (tv->v_type != VAR_DICT)
+ {
+ emsg(_(e_dictreq));
+ return FAIL;
+ }
+
+ if (dict_find(tv->vval.v_dict, (char_u *)"sort", -1) != NULL)
+ compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
If the key "sort" is not found, the result is unpredictable.
⬇️ Suggested change- compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
+ compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
+ else
+ {
+ *cmp = READDIR_SORT_BYTE;
+ return OK;
+ }
Or, should be an error?
In src/vim.h:
> @@ -2668,4 +2674,12 @@ long elapsed(DWORD start_tick); #define FSK_IN_STRING 0x04 // TRUE in string, double quote is escaped #define FSK_SIMPLIFY 0x08 // simplify <C-H> and <A-x> +#ifdef FEAT_EVAL +// Flags for the readdirex function, how to sort the result +# define READDIR_SORT_NONE 0 // do not sort +# define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default +# define READDIR_SORT_IC 2 // sort ignoreing case (strcasecmp)⬇️ Suggested change
-# define READDIR_SORT_IC 2 // sort ignoreing case (strcasecmp) +# define READDIR_SORT_IC 2 // sort ignoring case (strcasecmp)
> + The list will be sorted by name according to the current + locale, using the collation order.
Looks incorrect.
> @@ -7980,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
⬇️ Suggested change
- none do not sort (fastest method) + "none" do not sort (fastest method)
might be better (to make sure that this is a string)? (Same for the others.)
—
You are receiving this because you commented.
James McCoy
I especially find the S390 results strange.
The tests assume that the order in which the files are created are the order in which readdir() will return them. There are no guarantees about ordering. It's up to the individual filesystem implementation.
—
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/fileio.c:
> @@ -4872,7 +4887,7 @@ delete_recursive(char_u *name) exp = vim_strsave(name); if (exp == NULL) return -1; - if (readdir_core(&ga, exp, FALSE, NULL, NULL) == OK) + if (readdir_core(&ga, exp, FALSE, NULL, NULL, 0) == OK)
Indeed it actually changes behaviour, but then I thought, why should we actually sort the result, if we are going to delete it? And in addition it will be a small performance gain.
—
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/filepath.c:
> @@ -1424,7 +1424,7 @@ f_readdir(typval_T *argvars, typval_T *rettv)
expr = &argvars[1];
ret = readdir_core(&ga, path, FALSE, (void *)expr,
- (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem);
+ (expr->v_type == VAR_UNKNOWN) ? NULL : readdir_checkitem, 0);
yes makes sense, was an overview.
—
You are receiving this because you commented.
Christian Brabandt
> @@ -1469,6 +1469,32 @@ readdirex_checkitem(void *context, void *item)
return retval;
}
+ static int
+readdirex_dict_arg(typval_T *tv, int *cmp)
+{
+ char_u *compare;
+
+ if (tv->v_type != VAR_DICT)
+ {
+ emsg(_(e_dictreq));
+ return FAIL;
+ }
+
+ if (dict_find(tv->vval.v_dict, (char_u *)"sort", -1) != NULL)
+ compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
Yes, I change it.
—
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
In src/filepath.c:
> @@ -1469,6 +1469,32 @@ readdirex_checkitem(void *context, void *item)
return retval;
}
+ static int
+readdirex_dict_arg(typval_T *tv, int *cmp)
+{
+ char_u *compare;
+
+ if (tv->v_type != VAR_DICT)
+ {
+ emsg(_(e_dictreq));
+ return FAIL;
+ }
+
+ if (dict_find(tv->vval.v_dict, (char_u *)"sort", -1) != NULL)
+ compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
and make missing 'sort' key an error
Christian Brabandt
@chrisbra commented on this pull request.
In src/vim.h:
> @@ -2668,4 +2674,12 @@ long elapsed(DWORD start_tick); #define FSK_IN_STRING 0x04 // TRUE in string, double quote is escaped #define FSK_SIMPLIFY 0x08 // simplify <C-H> and <A-x> +#ifdef FEAT_EVAL +// Flags for the readdirex function, how to sort the result +# define READDIR_SORT_NONE 0 // do not sort +# define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default +# define READDIR_SORT_IC 2 // sort ignoreing case (strcasecmp)
thanks, will change
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -7980,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
yes, done
Christian Brabandt
> + The list will be sorted by name according to the current + locale, using the collation order.
indeed, changed it.
Christian Brabandt
> @@ -7931,7 +7939,7 @@ readdir({directory} [, {expr}]) *readdir()*
Can also be used as a |method|: >
GetDirName()->readdir()
<
-readdirex({directory} [, {expr}]) *readdirex()*
+readdirex({directory} [, {expr} [, {dict}]]) *readdirex()*
fixed
Christian Brabandt
@chrisbra commented on this pull request.
In src/fileio.c:
> @@ -4638,10 +4642,16 @@ create_readdirex_item(char_u *path, char_u *name)
compare_readdirex_item(const void *p1, const void *p2)
{
char_u *name1, *name2;
+
Christian Brabandt
> if (withattr) qsort((void*)gap->ga_data, (size_t)gap->ga_len, sizeof(dict_T*), compare_readdirex_item); else # endif sort_strings((char_u **)gap->ga_data, gap->ga_len); +
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -7980,6 +7989,24 @@ readdirex({directory} [, {expr}]) *readdirex()*
When {expr} is a function the entry is passed as the argument.
For example, to get a list of files ending in ".txt": >
readdirex(dirname, {e -> e.name =~ '.txt$'})
+<
+ The optional {dict} argument allows for further custom
+ values. Currently this is used to specify if and how sorting
+ should be performed. The dict can have the following members:
+
+ sort How to sort the result returned from the system.
+ Valid values are:
+ none do not sort (fastest method)
+ case sort case senstive
Christian Brabandt
@chrisbra commented on this pull request.
In src/fileio.c:
> @@ -4872,7 +4887,7 @@ delete_recursive(char_u *name) exp = vim_strsave(name); if (exp == NULL) return -1; - if (readdir_core(&ga, exp, FALSE, NULL, NULL) == OK) + if (readdir_core(&ga, exp, FALSE, NULL, NULL, 0) == OK)
I used the 0, because READDIR_SORT_NONE is undefined for tiny builts. I now define only READDIR_SORT_NONE in all cases, the other ifdefs only for EVAL feature. Not pretty.
Christian Brabandt
Thanks @k-takata and Gary for the reviews, I have adjusted it.
The tests assume that the order in which the files are created are the order in which readdir() will return them. There are no guarantees about ordering. It's up to the individual filesystem implementation.
@jamessan indeed the test assumes this. Any suggestions on how to improve the test? Or should we skip test for s390? BTW: What feature does need to be checked for that architecture? It is not completely obvious to me if we want to adjust tests specifically for that architecture.
K.Takata
@k-takata commented on this pull request.
In src/filepath.c:
> @@ -1469,6 +1469,32 @@ readdirex_checkitem(void *context, void *item)
return retval;
}
+ static int
+readdirex_dict_arg(typval_T *tv, int *cmp)
+{
+ char_u *compare;
+
+ if (tv->v_type != VAR_DICT)
+ {
+ emsg(_(e_dictreq));
+ return FAIL;
+ }
+
+ if (dict_find(tv->vval.v_dict, (char_u *)"sort", -1) != NULL)
+ compare = dict_get_string(tv->vval.v_dict, (char_u *)"sort", FALSE);
Currently, the optional dict only has the 'sort' key, making it mandatory may make sense.
But if another keys are added later, making it mandatory may not make sense.
K.Takata
@jamessan indeed the test assumes this. Any suggestions on how to improve the test?
Isn't the issue fixed by my suggested change?
Christian Brabandt
The CI tests aren't complete yet, but I assume some will still fail, because the newly added test depends on the order in which the files are written. And it looks like this can't be guaranteed everywhere.
K.Takata
@k-takata commented on this pull request.
In src/fileio.c:
> @@ -4872,7 +4887,7 @@ delete_recursive(char_u *name) exp = vim_strsave(name); if (exp == NULL) return -1; - if (readdir_core(&ga, exp, FALSE, NULL, NULL) == OK) + if (readdir_core(&ga, exp, FALSE, NULL, NULL, 0) == OK)
Ah, understood.
Then, how about always defining all of them?
James McCoy
@jamessan indeed the test assumes this. Any suggestions on how to improve the test? Or should we skip test for s390?
It has nothing to do with the architecture. You're just getting lucky on the other systems. The order of the items returned by readdir() is unspecified, so there shouldn't be any assertion on a specific ordering.
It's completely valid for XFS, btrfs, and ext4 to return the files in different orders than each other even if the same steps were taken to create the files.
Christian Brabandt
@jamessan Understood and I did not mean to hint that the sorting order is dependent on the architecture. Sorry for that. I did realize that the sorting order depends on the underlying filesystem. I guess what I actually did want to say where 2 questions:
- So we are lucky, that the sorting in all builds except for s390 built works, I'd like to keep the test and I'd like to have at least some test that makes sure this feature works. So even so the test works by coincidence only, what would be a better and reliable way to test? I suppose your answer to this question is: There is no way, so don't bother?
- How can I detect from within Vim that we are running on s390? So even if would like to disable this test for the S390 built, I have no idea how to test that from Vimscript. It's not
ebcdic, so I was just curious how to determine that we are running on S390? I did not see anything obvious at:h feature-list.
James McCoy
- So even so the test works by coincidence only, what would be a better and reliable way to test? I suppose your answer to this question is: There is no way, so don't bother?
I would say that the #{sort: 'none'} case can't be tested. This is exactly what's unspecified.
However, you can probably use that data to determine how the rest of the test assertions should look. Although it's still not guaranteed, I think it's far less likely that the ordering will change among different readdir() calls when there are no intervening changes to the filesystem.
- How can I detect from within Vim that we are running on s390?
There shouldn't be any reason to do that. There's nothing unique about an s390 system that's relevant to this test. The particulars of the Travis setup for CI might be relevant, but that has no bearing on any other s390 system.
If my above suggestion doesn't work, then maybe it would be worthwhile to change .travis.yml so it skips this test on s390. However, I would argue that's just hiding a flaky test that's going to cause problems downstream and cause people to waste time investigating false-negative test failures.
It's not ebcdic, so I was just curious how to determine that we are running on S390?
Right, because there's nothing different about how Vim is built. It's just a different architecture. There's nothing in Vim that let's you distinguish between i386, amd64, mips, m68k, etc.
K.Takata
I would say that the
#{sort: 'none'}case can't be tested.
Agree. It depends on its underlying filesystem, not on OS.
Gary Johnson
@ghgary commented on this pull request.
In src/testdir/test_functions.vim:
> @@ -1903,6 +1903,54 @@ func Test_readdirex()
eval 'Xdir'->delete('rf')
endfunc
+func Test_readdirex_sort()
+ CheckUnix
+ CheckNotBSD
+ if !(has("osxdarwin") || has("osx") || has("macunix"))
+ let _collate = v:collate
+ call mkdir('Xdir2')
+ call writefile(['1'], 'Xdir2/README.txt')
+ call writefile(['2'], 'Xdir2/Readme.txt')
+ call writefile(['3'], 'Xdir2/readme.txt')
+
+ " Skip tests on Mac OS X (does not allow several files with different caseing)
Spelling: caseing -> casing
Bram Moolenaar
The Implementation looks OK now, thanks.
We should have a test that also works on Mac, using different file names. Also, the test still fails on s390.
Tony Mechelynck
The Implementation looks OK now, thanks.
We should have a test that also works on Mac, using different file names. Also, the test still fails on s390.
Maybe do the test differently if has('ebcdic')? (zArchitecture, but not Linux on Z, usually uses EBCDIC encoding IIUC.)
K.Takata
@k-takata commented on this pull request.
In src/testdir/test_functions.vim:
> + if !(has("osxdarwin") || has("osx") || has("macunix"))
+ let _collate = v:collate
+ call mkdir('Xdir2')
+ call writefile(['1'], 'Xdir2/README.txt')
+ call writefile(['2'], 'Xdir2/Readme.txt')
+ call writefile(['3'], 'Xdir2/readme.txt')
+
+ " 1) default
+ let files = readdirex('Xdir2')->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort using default')
+
+ " 2) no sorting
+ let files = readdirex('Xdir2', 1, #{sort: 'none'})->map({-> v:val.name})
+ " not possible to assert the order in which the files appear, so just make
+ " sure it matches in any order.
+ call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted')
How about comparing after sorting?
⬇️ Suggested change- call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted')
+ call assert_match(['README.txt', 'Readme.txt', 'readme.txt'], sort(files), 'unsorted')
In src/testdir/test_functions.vim:
> + let files = readdirex('Xdir2')->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort using default')
+
+ " 2) no sorting
+ let files = readdirex('Xdir2', 1, #{sort: 'none'})->map({-> v:val.name})
+ " not possible to assert the order in which the files appear, so just make
+ " sure it matches in any order.
+ call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted')
+
+ " 3) sort by case (same as default)
+ let files = readdirex('Xdir2', 1, #{sort: 'case'})->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort by case')
+
+ " 4) sort by ignoring case
+ let files = readdirex('Xdir2', 1, #{sort: 'icase'})->map({-> v:val.name})
+ call assert_equal(['README.txt', 'readme.txt', 'Readme.txt'], files, 'sort by icase')
I don't think the order of the files can be determined here.
'README.txt', 'readme.txt' and 'Readme.txt' have the same priority, then they can be sorted in any order.
Maybe, a good additional test is e.g. using 'a.txt', 'b.txt' and 'Z.txt':
case -> 'Z.txt', 'a.txt', 'b.txt'
icase -> 'a.txt', 'b.txt', 'Z.txt'
This test can be also executed on Windows and macOS.
In src/testdir/test_functions.vim:
> + call assert_equal(['README.txt', 'readme.txt', 'Readme.txt'], files, 'sort by icase')
+
+ " 5) Default Collation
+ let collate = v:collate
+ lang collate C
+ let files = readdirex('Xdir2', 1, #{sort: 'collate'})->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort by C collation')
+
+ " 6) Collation de_DE
+ " Switch locale, this may not work on the CI system, if the locale isn't
+ " available
+ try
+ lang collate de_DE
+ let files = readdirex('Xdir2', 1, #{sort: 'collate'})->map({-> v:val.name})
+ call assert_equal(['readme.txt', 'Readme.txt', 'README.txt'], files, 'sort by de_DE collation')
+ catch
How about adding a skip message?
⬇️ Suggested change- catch + catch + throw 'Skipped: de_DE collation is not available'
In src/vim.h:
> +#ifdef HAVE_STRCOLL + # define STRCOLL(d, s) strcoll((char *)(d), (char *)(s)) +# else + # define STRCOLL(d, s) strcmp((char *)(d), (char *)(s)) +# endif⬇️ Suggested change
-#ifdef HAVE_STRCOLL - # define STRCOLL(d, s) strcoll((char *)(d), (char *)(s)) -# else - # define STRCOLL(d, s) strcmp((char *)(d), (char *)(s)) -# endif +#ifdef HAVE_STRCOLL +# define STRCOLL(d, s) strcoll((char *)(d), (char *)(s)) +#else +# define STRCOLL(d, s) strcmp((char *)(d), (char *)(s)) +#endif
In src/testdir/test_functions.vim:
> + " Skip tests on Mac OS X (does not allow several files with different casing)
+ if !(has("osxdarwin") || has("osx") || has("macunix"))
Same for Cygwin.
⬇️ Suggested change- " Skip tests on Mac OS X (does not allow several files with different casing)
- if !(has("osxdarwin") || has("osx") || has("macunix"))
+ " Skip tests on Mac OS X and Cygwin (does not allow several files with different casing)
+ if !(has("osxdarwin") || has("osx") || has("macunix") || has("win32unix"))
Maybe it's better to add a skip message?
In src/filepath.c:
> + semsg(_(e_no_dict_key), "sort"); + return FAIL;
I'm still wondering if this error is really needed. #6229 (comment)
In src/vim.h:
> +# define READDIR_SORT_NONE 0 // do not sort +# define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default +# define READDIR_SORT_IC 2 // sort ignoring case (strcasecmp) +# define READDIR_SORT_COLLATE 3 // sort according to collation (strcoll)⬇️ Suggested change
-# define READDIR_SORT_NONE 0 // do not sort -# define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default -# define READDIR_SORT_IC 2 // sort ignoring case (strcasecmp) -# define READDIR_SORT_COLLATE 3 // sort according to collation (strcoll) +#define READDIR_SORT_NONE 0 // do not sort +#define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default +#define READDIR_SORT_IC 2 // sort ignoring case (strcasecmp) +#define READDIR_SORT_COLLATE 3 // sort according to collation (strcoll)
Tony Mechelynck
@tonymec commented on this pull request.
In src/testdir/test_functions.vim:
> + let files = readdirex('Xdir2')->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort using default')
+
+ " 2) no sorting
+ let files = readdirex('Xdir2', 1, #{sort: 'none'})->map({-> v:val.name})
+ " not possible to assert the order in which the files appear, so just make
+ " sure it matches in any order.
+ call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted')
+
+ " 3) sort by case (same as default)
+ let files = readdirex('Xdir2', 1, #{sort: 'case'})->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort by case')
+
+ " 4) sort by ignoring case
+ let files = readdirex('Xdir2', 1, #{sort: 'icase'})->map({-> v:val.name})
+ call assert_equal(['README.txt', 'readme.txt', 'Readme.txt'], files, 'sort by icase')
Yeah, but on EBCDIC systems, in byte-value sorting, lowercase comes before uppercase and digits come last, (a-i 0x81-0x89, j-r 0x91-0x99, s-z 0xA2-0xA9, A-I 0xC1-0xC9, J-R 0xD1-0xD9, S-Z 0xE2-0xE9, digits 0xF0-0xF9, with punctuation characters in-between), so you may need to test for has('ebcdic') and if true compare A.txt, B.txt and z.txt (and if sorted on non-case-folded byte values, z will come before A).
Best regards,
Tony.
James McCoy
Also, the test still fails on s390.
Maybe do the test differently if has('ebcdic')?
has('ebcdic') is irrelevant for the Linux s390 build.
James McCoy
I would say that the
#{sort: 'none'}case can't be tested. This is exactly what's unspecified.However, you can probably use that data to determine how the rest of the test assertions should look.
I was thinking of something like below. This does pass on Travis.
diff --git a/src/testdir/test_functions.vim b/src/testdir/test_functions.vim index 2316ac01d13..2463e3e5f67 100644 --- a/src/testdir/test_functions.vim +++ b/src/testdir/test_functions.vim @@ -1942,21 +1942,23 @@ func Test_readdirex_sort() " 1) default let files = readdirex('Xdir2')->map({-> v:val.name}) + let default = copy(files) call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort using default') " 2) no sorting let files = readdirex('Xdir2', 1, #{sort: 'none'})->map({-> v:val.name}) + let unsorted = copy(files) " not possible to assert the order in which the files appear, so just make " sure it matches in any order. call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted') " 3) sort by case (same as default) let files = readdirex('Xdir2', 1, #{sort: 'case'})->map({-> v:val.name}) - call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort by case') + call assert_equal(default, files, 'sort by case') " 4) sort by ignoring case let files = readdirex('Xdir2', 1, #{sort: 'icase'})->map({-> v:val.name}) - call assert_equal(['README.txt', 'readme.txt', 'Readme.txt'], files, 'sort by icase') + call assert_equal(unsorted->sort('i'), files, 'sort by icase') " 5) Default Collation let collate = v:collate
—
Christian Brabandt
thanks everybody, will rework and apply the suggestions later today.
Christian Brabandt
@chrisbra commented on this pull request.
In src/testdir/test_functions.vim:
> + if !(has("osxdarwin") || has("osx") || has("macunix"))
+ let _collate = v:collate
+ call mkdir('Xdir2')
+ call writefile(['1'], 'Xdir2/README.txt')
+ call writefile(['2'], 'Xdir2/Readme.txt')
+ call writefile(['3'], 'Xdir2/readme.txt')
+
+ " 1) default
+ let files = readdirex('Xdir2')->map({-> v:val.name})
+ call assert_equal(['README.txt', 'Readme.txt', 'readme.txt'], files, 'sort using default')
+
+ " 2) no sorting
+ let files = readdirex('Xdir2', 1, #{sort: 'none'})->map({-> v:val.name})
+ " not possible to assert the order in which the files appear, so just make
+ " sure it matches in any order.
+ call assert_match('^\c\(readme\C\.txt\)\{3\}$', join(files, ''), 'unsorted')
And here I am, thinking I was overly clever using assert_match() :/
Thanks, good suggestion 👍
Christian Brabandt
@chrisbra commented on this pull request.
In src/filepath.c:
> + semsg(_(e_no_dict_key), "sort"); + return FAIL;
I don't mind either way. I leave it to @brammool, the error message also needs a proper number. I am on the fence, but I slightly prefer, if Vim would error if I had a typo like readdirex(..., 1, #{sorta: 'case'}.
Christian Brabandt
@chrisbra commented on this pull request.
In src/vim.h:
> +# define READDIR_SORT_NONE 0 // do not sort +# define READDIR_SORT_BYTE 1 // sort by byte order (strcmp), default +# define READDIR_SORT_IC 2 // sort ignoring case (strcasecmp) +# define READDIR_SORT_COLLATE 3 // sort according to collation (strcoll)
Opps, where did the space come from...
Christian Brabandt
okay reworked.
Dominique Pellé
@dpelle commented on this pull request.
> Print the current language (aka locale). With the "messages" argument the language used for messages is printed. Technical: LC_MESSAGES. With the "ctype" argument the language used for character encoding is printed. Technical: LC_CTYPE. With the "time" argument the language used for strftime() is printed. Technical: LC_TIME. + With the "collate" argument the language used for + collation order is printed. Technical: LC_COLLATE.
This line uses spaces for indentation whereas nearby line use tabs (inconsistent).
K.Takata
@k-takata commented on this pull request.
In src/testdir/test_functions.vim:
> @@ -1929,6 +1929,86 @@ func Test_readdirex()
eval 'Xdir'->delete('rf')
endfunc
+func Test_readdirex_sort()
+ CheckUnix
+ CheckNotBSD
Is this check still needed? Doesn't this test work on BSDs?
K.Takata
> - CheckUnix - CheckNotBSD
Sorry, I meant removing only CheckNotBSD. I think CheckUnix is still needed.
Christian Brabandt
Christian Brabandt
@chrisbra commented on this pull request.
In src/testdir/test_functions.vim:
> @@ -1929,6 +1929,86 @@ func Test_readdirex()
eval 'Xdir'->delete('rf')
endfunc
+func Test_readdirex_sort()
+ CheckUnix
+ CheckNotBSD
I removed.
Christian Brabandt
Christian Brabandt
@chrisbra commented on this pull request.
> Print the current language (aka locale). With the "messages" argument the language used for messages is printed. Technical: LC_MESSAGES. With the "ctype" argument the language used for character encoding is printed. Technical: LC_CTYPE. With the "time" argument the language used for strftime() is printed. Technical: LC_TIME. + With the "collate" argument the language used for + collation order is printed. Technical: LC_COLLATE.
thanks, fixed it as well.
Christian Brabandt
@chrisbra commented on this pull request.
In src/testdir/test_functions.vim:
> - CheckUnix - CheckNotBSD
Hm, I thought we do not need this anymore. Anyhow, I re-added it back
K.Takata
This line needs to be updated: https://github.com/vim/vim/pull/6229/files#diff-bea881dfa9626bab7141337b0fcdb23eR7921
Christian Brabandt
This line needs to be updated: https://github.com/vim/vim/pull/6229/files#diff-bea881dfa9626bab7141337b0fcdb23eR7921
Which one? It's not clear from that link.
K.Takata
Oh, sorry. The description of readdir():
https://github.com/vim/vim/blob/3c741f50c6132fac1dd0992b56839117f281e1ce/runtime/doc/eval.txt#L7921
It should be
The list will by default be sorted by name (case sensitive),
see below for changing the sorting.
like readdirex().
And readdirex() should be also updated?
https://github.com/vim/vim/blob/3c741f50c6132fac1dd0992b56839117f281e1ce/runtime/doc/eval.txt#L7978-L7979
Not "below" now, I think.
codecov[bot]
Codecov Report
Merging #6229 into master will decrease coverage by
0.00%.
The diff coverage is87.27%.
@@ Coverage Diff @@ ## master #6229 +/- ## ========================================== - Coverage 87.48% 87.47% -0.01% ========================================== Files 143 143 Lines 158424 158475 +51 ========================================== + Hits 138590 138626 +36 - Misses 19834 19849 +15
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/evalfunc.c | 96.37% <ø> (ø) |
|
| src/evalvars.c | 95.98% <ø> (ø) |
|
| src/filepath.c | 86.23% <77.77%> (-0.19%) |
⬇️ |
| src/fileio.c | 84.85% <93.75%> (+0.04%) |
⬆️ |
| src/cmdexpand.c | 92.96% <100.00%> (+<0.01%) |
⬆️ |
| src/ex_cmds2.c | 90.85% <100.00%> (+0.14%) |
⬆️ |
| src/gui.c | 63.37% <0.00%> (-0.51%) |
⬇️ |
| src/mbyte.c | 79.62% <0.00%> (-0.25%) |
⬇️ |
| src/sign.c | 94.77% <0.00%> (-0.18%) |
⬇️ |
| src/window.c | 89.55% <0.00%> (-0.11%) |
⬇️ |
| ... and 10 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update b340bae...51bdd1c. Read the comment docs.
Christian Brabandt
all CI pass now
Christian Brabandt
I can squash into a single commit and force-push, if this is helpful.
Bram Moolenaar
I suppose it's OK to include now. Thanks for fine-tuning it.
Yegappan Lakshmanan
I can squash into a single commit and force-push, if this is helpful.
vim-dev ML
—
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Yegappan Lakshmanan
Thanks, done: #6278
vim-dev ML
>
>
>
Thanks for adding the tests.
After adding a lot of tests we have reached 87.5% code coverage recently.
The goal is to reach 90%. To maintain this, we need to make sure that the
newly introduced code doesn't reduce the code coverage.
Regards,
Yegappan
—
You are receiving this because you are subscribed to this thread.
lacygoill
I think this todo item is no longer needed:
https://github.com/vim/vim/blob/caf73dcfade0a435ea3f989285b43f07c40c9948/runtime/doc/todo.txt#L282
—
You are receiving this because you commented.
Bram Moolenaar
I'll update the todo list