Open adigitoleo opened 4 months ago
Numbers were used when signals were added to an old luvit version. After luv was split into its own project, signals were reintroduced using uppercase strings. This implementation was eventually changed to using lowercase strings and moved to a dedicated constants file where it remains, along with other constants that are handled as lowercase strings.
This was all 10+ years ago and before I joined, so I can't speak for those who made the change, but luv generally uses strings instead of numbers where possible. I assume that this is for consistency with Lua's recommendation to use strings for options.
The usual convention in Lua libraries is to use strings instead of numbers to select options.
In any case, the return type is documented as a string, so we could not easily change this if we wanted. I think that we can do better to document string constants that are accepted and expected, though.
I do agree that the existence of numbers in the constants
table is confusing. Perhaps we could also expose the strings in a constants table, or expose the string_to_num
and/or num_to_string
converters.
A simple backwards compatible solution would be to pass a second parameter to the callback that contains the integer representation.
Lua's recommendation to use strings for options
OK, this makes sense in a way. The change to lowercase is still strange considering what kill -l
and such usually output, but what's done is done.
There seem to be three representations in the current API:
signum: string
for the callback functions already mentionedsignal: integer
for the on_exit
handler in spawn
signum: integer or string or nil
for process_kill
and kill
It seems like relaxing them all to accept either an integer or a string could be done in a backwards-compatible way. My opinion is that this is preferable to defining a second constants table + converters, but it doesn't exclude doing that as well. I suggest that the parameter be called signal
across the whole API, not signum
, and that this should accept either a numeric value or the lowercase string (from a list of possible values that could be listed somewhere in the docs). To avoid breaking changes, process_kill
and kill
can also accept nil
(i.e. the signal arg is optional). If that sounds reasonable I can work on a PR over the weekend.
It seems like relaxing them all to accept either an integer or a string could be done in a backwards-compatible way
I'm not sure I understand what you're proposing. There's no way to infer what type the callback is expecting, and process_kill
and kill
already accept the same types, so I'm not sure what you mean by 'relaxing them all'.
EDIT: Maybe you mean something like "if the signal is passed to the function as an integer, then the callback should be called with an integer"? But that would still be a breaking change, and it can't apply to spawn
.
To give you an idea of the change I had in mind, for the callbacks, the backwards compatible approach would be something like:
For uv.spawn
's on_exit
callback, the parameters would now be:
code
: integer
signum
: integer
(renamed parameter)signame
: string
(new parameter)For uv.signal_start
and uv.signal_start_oneshot
's callbacks, the parameters would now be:
signame
: string
(renamed parameter)signum
: integer
(new parameter)It's a bit unfortunate that these parameters would have to be in a different order between spawn
/signal_start
, but not sure what alternative we have.
if the signal is passed to the function as an integer, then the callback should be called with an integer
I confused myself a bit but yes, something like that is what I had in mind, and the same for strings. But if it doesn't work for spawn
then no worries. I guess what you are proposing is the only real compromise in that case. Maybe this just boils down to a documentation issue in the end, I guess having the list of possible strings referenced is the most obvious thing.
Changing to signame
is good, though I would recommend sigstr
or sigstring
if either of those provide any extra assurance that the value is e.g. "sigint"
instead of "SIGINT"
.
We probably should document that the value can technically be nil
for unknown signals as supported by lua_pushstring
.
We can document the list of supported strings similarly to how we document FS modes. Since signals appear in multiple places, we can add a new section dedicated to constants to avoid duplication, and we can point the FS modes and other constants to there.
Adding the new parameters is also fine, if that helps things. The order difference is indeed unfortunate, though.
With #713, we now have documented the behavior encountered in this issue. As you can see by this change, there are places other than signals where the library optionally accepts lowercase strings or integer constants, but only returns lowercase strings. You may also find other places that only accept lowercase strings, or integer constants, but not both. When I get a chance, I'll make a list of all of these encounters, then we can think about how we can maximize UX without complicating library behavior. After documenting things, my belief at this point is that accepting multiple input types is already complicated enough, and providing multiple output types would be impractical in certain cases, but I'm open minded.
Thanks, the list should help quite a lot and I think the simple examples are nice too. I'll do my best to keep an eye on the constants -> strings mapping in that file in case I notice any changes that update the constants table but miss the docs. These changes should propagate to :h luvref
in NeoVim and that should help users like me to notice the difference in signal representations between luv and e.g. vim.system
.
accepting multiple input types is already complicated enough, and providing multiple output types would be impractical in certain cases
I'm sympathetic to this concern, I wasn't aware of the lua string recommendation when creating the issue and that has softened my opinion. I'm happy for this to be closed in favour of more targeted future UX proposals.
Unless there is a strong reason for the current behavior, I propose that the callback which is given as the third argument to
signal_start
andsignal_start_oneshot
should receive/propagate the signal as a number. Currently, the signal is propagated as a lowercase string. This is confusing for several reasons:int
, see heresignum
, implying a numeric representationuv.constants.SIGINT
, and other signals, is an integerThis example demonstrates the confusion for a NeoVim user. Sure, the outputs of
vim.system
have no obligation to be consistent with luv, and vice versa, but it would make things simpler for us. Requires NeoVim 0.10 for thevim.uv
namespace. Note also thatvim.system
doesn't allow SIGINT hooks (only atimeout
one) so this use case is more than hypothetical.