[RDY] API: nvim_get_commands #8060

nimitbhardwaj · 2018-02-24T10:17:06Z

Add the nvim_get_commands API function,
Returns the dictionary of commands.

nimitbhardwaj · 2018-02-24T10:18:06Z

Presently just a prototype, means it works list the command successfully, but some necessary things like documentation, tests are omitted, will be added soon, It first needs to be reviewed

bfredl · 2018-02-24T12:50:14Z

src/nvim/ex_docmd.c

+  return ucmds.ga_len;
+}
+
+Dictionary get_dict_command(int i) {


I think you can have the function return an Array directly, then separate n_get_commands is not needed.

bfredl · 2018-02-24T12:52:42Z

src/nvim/ex_docmd.c

+Dictionary get_dict_command(int i) {
+  Dictionary dic = ARRAY_DICT_INIT;
+  ucmd_T *cmd = USER_CMD(i);
+  PUT(dic, "uc_name", STRING_OBJ(cstr_to_string((const char *)cmd->uc_name)));


these are the internal names, we don't want them in external API. Better to use terms mentioned in documentation :help command.

nimitbhardwaj · 2018-02-25T08:57:49Z

Made the necessary adjustments by seeing the uc_list, but if there is a room for adjustments I would be happy to do so, and after that my work is adding documentation and testing

nimitbhardwaj · 2018-02-25T08:58:39Z

And during testing, I want to know how to do its testing in lua, means we apply command and get output?

nimitbhardwaj · 2018-02-25T12:19:00Z

Is it right or something to be done.

bfredl · 2018-02-25T17:46:16Z

check test/functional/api/vim_spec.lua (and rest of api/ directory) for tests of API function. Tests for keymap api/keymap_spec.lua might be inspiration.

Also just like nvim_get_keymap has nvim_buf_get_keymap, we will need nvim_buf_get_commands variant for buffer local commands. (buf->b_ucmds)

nimitbhardwaj · 2018-02-26T07:49:09Z

OK, this will be done

nimitbhardwaj · 2018-02-26T08:11:53Z

Now I make the tests

justinmk · 2018-02-26T09:39:45Z

src/nvim/ex_docmd.c

+    PUT(dic, "ScriptID", INTEGER_OBJ(cmd->uc_scriptID));
+
+    // Compl Arg
+    PUT(dic, "Compl Arg",


key names never have whitespace and are never capitalized.

Was thinking of Compl Arg same, didn't get the feel of a coder here, I change it

justinmk · 2018-02-26T09:42:18Z

src/nvim/ex_docmd.c

+/// @param[in] buf the specified buffer, if NULL then global commands required
+///
+/// @return The array of the commands and the information of each command
+ArrayOf(Dictionary) get_commands(buf_T *buf)


Not sure if it's the best convention, but our current convention (for internal wrappers that convert internal structures to Array) is to name this commands_array.

Existing examples: mode_style_array, keymap_array. Until a better convention is found, we should always do the same thing, to avoid having numerous inconsistent patterns.

nimitbhardwaj · 2018-02-26T18:25:34Z

Made the basic tests, by understanding the test of get_keymap, may be more things are to be added after it, but its basic of the tests I made

justinmk · 2018-02-27T07:34:03Z

src/nvim/ex_docmd.c


    // Defination
-    PUT(dic, "Defination",
+    PUT(dic, "defination",


typo, "definition" (also the comment above it, which probably should just be deleted)

Okk, so, I think my english is sooo bad, I generally used defination, it was wrong, 😆

And yes, I have to delete the comments?
I thought they are useful to see the code tidy.

Today, I am laughing on myself, I didn't even know the spelling of Definition, how poor, haha haha 🤣

@nimitbhardwaj No worries!

And yes, I have to delete the comments?

Trivial comments that restate the obvious aren't needed. But if they describe something non-obvious, or if they describe a group of lines, then they might be useful.

nimitbhardwaj · 2018-02-27T09:17:17Z

I edited the typo, but should I remove all comments, that were added, I added them because I thought they provide the info and somewhat beautify the code.

nimitbhardwaj · 2018-02-27T09:41:39Z

src/nvim/ex_docmd.c

+
+    // Complete
+    for (j = 0; command_complete[j].expand != 0; j++) {
+      if (cmd->uc_compl == command_complete[j].expand) {


This operation can be made more fast, command_complete is just a static array, we manually inserted the items in it, if the items there are added in sorted order then by using binary search O(log(n)) complexity can be achieved instead of O(n), so possibliity can be just arange the items in array, replace linear search with bin search

I think the proper refactor would be to define the complete strings next to the constants, as a dense array of string constants (with NULL if some constant lacks a string). Probably, move EXPAND_COMMANDS etc enum from vim.c and command_complete array both to ex_cmds_defs.h. Compare to typedef enum hlf_T and hlf_names in highlight_defs.h. Maybe an accessor function should be used, to do bounds checking (negative index should return NULL string).

How about this possibility, for array command_complete, I add {INT_MIN, NULL} at the end, this element can be used to serve the end

You were asking for buffer?, means if negetive buffer number is sent?

Didn't get it

It is used by ExpandGeneric which sorts the results anyway, so reordering them wouldn't be an issue.

yes, ExpandGeneric uses it, I also found it, so true, reordering is best way here

I reorder them according to the value of the EXPAND_

I sorted the list of commands according to the values of EXPAND_, now I want to test if it will be correct, will get_user_cmd_complete function pose no difficulty, where is its test

Sorting the lines in reverse sorted is better, so then we have not to care for the 0 as the last element which acts as comparator for end, and also useful in binary search

nimitbhardwaj · 2018-02-27T09:42:28Z

src/nvim/ex_docmd.c

+    // Address
+    for (j = 0; addr_type_complete[j].expand != -1; j++) {
+      if (addr_type_complete[j].expand != ADDR_LINES
+          && addr_type_complete[j].expand == cmd->uc_addr_type) {


Here too is possibility

nimitbhardwaj · 2018-02-27T12:31:37Z

I edited the array command_complete, this array is now sorted in descending order according to .expand (descending because, last element should be {0, NULL} so) and I add the binary search for case of calculating complete.
But this is under assumption that everything will be okay, no order specificness was there, if any error is encountered, I will try to fix it or in case of some misery, I will rollback this commit.
Please check it, its start, if needed, I can also replace linear search with bin search not a big deal

bfredl · 2018-02-27T12:49:15Z

Sorry, but binary search of enum value in static array doesn't make any sense. Enum lookup array is both simpler and faster.

nimitbhardwaj · 2018-02-27T13:01:45Z

Okk, but how about that function, that will not work, because loop is used there, what about it

bfredl · 2018-02-27T13:02:58Z

what function?

nimitbhardwaj · 2018-02-27T13:09:27Z

get_user_cmd_complete, I reordered the array, so it don't pose issue in the function.
So making lookup will be a difficulty in iterating the elements when ith one is needed

bfredl · 2018-02-27T13:21:23Z

As I said on the line comment, that function can be supported by making the EXPAND_ kinds with a name first in the enum, then the NULL check in ExpandGeneric will work to get the right number of strings. The point of this refactor would be that it is both simpler and faster, while binary search of enum value adds more complexity for very small speed gain in not quite hot code path.

nimitbhardwaj · 2018-02-27T13:23:40Z

I completely understand this, I didn't use much of lookup array in C before, it is C99 addition, now I am implementing it, yes, its easier.

bfredl · 2018-02-27T13:27:03Z

The syntax might be new, but the pattern is not. For instance an "X macro" (with two args) could be used in older version on C to keep enum and string order matching.

nimitbhardwaj · 2018-02-27T18:46:00Z

I made it, the lookup method, this works correctly, some mistakes may be there, some concept of NULL pointers, so I request for a review.

bfredl · 2018-02-27T19:14:40Z

src/nvim/ex_docmd.c

@@ -5815,7 +5815,7 @@ char_u *get_user_cmd_complete(expand_T *xp, int idx)
  }
  char *cmd_compl = get_command_complete(idx);
  if (cmd_compl == NULL) {
-    return "\000";


just NULL. Thus the if can be removed.

No, in ExpandGeneric function, there is a check for NUL,

as I investigated, NUL is '\000', so

which is same as "", thus made the small change

NUL and NULL are used differently

you're right, it should work.

bfredl · 2018-03-02T09:10:13Z

src/nvim/ex_docmd.c

+    case(EXTRA|NEEDARG):       Arg[0] = '+'; break;
+    case(EXTRA|NOSPC|NEEDARG): Arg[0] = '1'; break;
+    }
+    PUT(dic, "argument", STRING_OBJ(cstr_to_string((const char *)Arg)));


bfredl · 2018-03-02T09:15:44Z

src/nvim/ex_docmd.c

+    PUT(dic, "script_id", INTEGER_OBJ(cmd->uc_scriptID));
+
+    // Compl Arg
+    PUT(dic, "compl_arg",


this key should also only be present if the argument was not NULL. Also it could be called "complete_arg", and this code could be moved just below the "complete" code above.

bfredl · 2018-03-02T09:18:40Z

src/nvim/ex_docmd.c

+    for (j = 0; addr_type_complete[j].expand != -1; j++) {
+      if (addr_type_complete[j].expand != ADDR_LINES
+          && addr_type_complete[j].expand == cmd->uc_addr_type) {
+        PUT(dic, "address",


"addr" (we typically prefer spelled out form when we add new keywords, but addr is already established in :command -addr)

ok this is better

bfredl · 2018-03-02T09:18:58Z

src/nvim/ex_docmd.c

+        break;
+      }
+    }
+    if (!ck) {


bfredl · 2018-03-02T09:21:33Z

test/functional/api/command_spec.lua

+
+describe('get_commands', function()
+  local cmd_string = 'command Hello echo "Hello World"'
+  local cmd_string2 = 'command Pwd pwd'


Why not add a bit more attributes like complete,address etc here? (they don't need to make sense, just to exercise the code)

this too done, added 2 more tests for this, exploiting all features of command

nimitbhardwaj · 2018-03-02T10:31:17Z

So I did it

nimitbhardwaj · 2018-04-15T07:25:29Z

I rebased it too

justinmk · 2018-04-15T15:23:53Z

Does this return builtin commands or only user-defined commands? If only user-defined commands, we should add a parameter to allow enhancement later without needing to add a new API function (or break the existing one; see :help api-contract). e.g.:

nvim_get_commands(Boolean builtin)

and only builtin=false is supported until future enhancement.

nimitbhardwaj · 2018-04-15T15:36:44Z

It displays perhaps user defined commands, commands like echo, command, substitute etc are not displayed, so only the user defined commands are available, basically I used the output of the :command to generate the list.

And for the case of adding a parameter builtin I add it, and also I add a todo for it to create the support for the builtin commands in future, it presently only support the commands entered by the user, or commands declared with the command

bfredl · 2018-04-15T15:51:19Z

An alternative is to later on include builtin commands in metadata. Yes, it is called "API metadata" for the moment, but I see no reason not to generalize it to other compile-time constant data of similar kind.

nimitbhardwaj · 2018-04-15T15:58:24Z

Yes, generalization of this seems messy, because I thought that there would be 3 options, so adding just boolean is not so good, 3 options are, builtin, user-def and both builtin and user-def.
I think there should firstly be the implementation to get the list of the builtin commands, then generalization would be much better.
How about this idea @justinmk and @bfredl, this is what I think about it
Otherwise adding a boolean is not so difficult, just I have to change its definations and some tests

nimitbhardwaj · 2018-04-15T16:06:10Z

Although I have done the changes, but I would like to add the builtinCommand_array() function in future

Please have a check if its correct.

justinmk · 2018-04-15T17:09:44Z

For now, let it take a dictionary which currently is ignored:

nvim_get_commands(Dictionary opts)

Although I agree with @bfredl , I think we should plan to support returning builtin commands from nvim_get_command in the future, for "ergonomics".

nimitbhardwaj · 2018-04-15T17:18:58Z

OK, we need to take the dictionary which will be ignored completely. I got it. The dictionary will hold the metadata in future, its good too.

nimitbhardwaj · 2018-04-15T17:41:40Z

👍

justinmk · 2018-05-10T23:30:03Z

Merged in #8375

bfredl reviewed Feb 24, 2018

View reviewed changes

nimitbhardwaj force-pushed the nvim_get_commands branch from 522c18a to 54a16f5 Compare February 25, 2018 08:56

nimitbhardwaj changed the title ~~Add the nvim_get_commands function~~ [RFC] Add the nvim_get_commands function Feb 25, 2018

marvim added the RFC label Feb 25, 2018

justinmk reviewed Feb 26, 2018

View reviewed changes

justinmk reviewed Feb 27, 2018

View reviewed changes

nimitbhardwaj commented Feb 27, 2018

View reviewed changes

nimitbhardwaj force-pushed the nvim_get_commands branch from 3ee987b to 390f7f2 Compare February 27, 2018 18:44

bfredl reviewed Feb 27, 2018

View reviewed changes

bfredl reviewed Mar 2, 2018

View reviewed changes

justinmk changed the title ~~[RFC] Add the nvim_get_commands function~~ [RFC] API: nvim_get_commands Mar 3, 2018

justinmk mentioned this pull request Mar 3, 2018

API: nvim_list_uis #8004

Closed

nimitbhardwaj changed the title ~~[RFC] API: nvim_get_commands~~ [RFY] API: nvim_get_commands Mar 23, 2018

nimitbhardwaj changed the title ~~[RFY] API: nvim_get_commands~~ [RDY] API: nvim_get_commands Mar 23, 2018

marvim added RDY and removed RFC labels Mar 23, 2018

justinmk added this to the 0.2.3 milestone Mar 26, 2018

nimitbhardwaj force-pushed the nvim_get_commands branch from 88570a2 to 967d3e4 Compare April 15, 2018 07:24

nimitbhardwaj force-pushed the nvim_get_commands branch from 967d3e4 to 566a5eb Compare April 15, 2018 16:05

Add the nvim_get_commands function

4c9673c

nimitbhardwaj force-pushed the nvim_get_commands branch from 566a5eb to 4c9673c Compare April 15, 2018 17:41

justinmk added the gsoc community: Google Summer of Code project label Apr 24, 2018

justinmk mentioned this pull request May 10, 2018

API: nvim_get_commands #8375

Merged

justinmk closed this May 10, 2018

Uh oh!

[RDY] API: nvim_get_commands #8060

[RDY] API: nvim_get_commands #8060

Uh oh!

Conversation

nimitbhardwaj commented Feb 24, 2018

Uh oh!

nimitbhardwaj commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimitbhardwaj commented Feb 25, 2018

Uh oh!

nimitbhardwaj commented Feb 25, 2018

Uh oh!

nimitbhardwaj commented Feb 25, 2018

Uh oh!

bfredl commented Feb 25, 2018

Uh oh!

nimitbhardwaj commented Feb 26, 2018

Uh oh!

nimitbhardwaj commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimitbhardwaj commented Feb 26, 2018

Uh oh!

justinmk Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimitbhardwaj commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimitbhardwaj commented Feb 27, 2018

Uh oh!

bfredl commented Feb 27, 2018

Uh oh!

nimitbhardwaj commented Feb 27, 2018

justinmk Feb 26, 2018 •

edited

Loading

justinmk Feb 27, 2018 •

edited

Loading

justinmk Feb 27, 2018 •

edited

Loading