Re: [PATCH] zsh/random module

[Clinton Bunch <cdb_zsh@xxxxxxxxxxx>]

On 11/3/2022 10:17 PM, dana wrote:
> On Wed 2 Nov 2022, at 12:13, Clinton Bunch wrote:
> > I'd appreciate if someone would test it on a few other platforms and try
> > to break it.  I tried everything I could think of, but...
> I like this idea, i've wanted to do something like it for a long time. It
> seems to work OK for me on Catalina as well
>
> Here's my feedback. Sorry if any of this came up in previous discussions. I
> also didn't look super closely at the code, just skimmed it for high-level
> stuff, so idk about the security, portability, &c.
>
>
> API/behaviour:
>
> The default length behaviour with -i is weird to me. I get that there's a
> certain internal consistency to it, but i feel like 95% of the time if you
> want a random integer you only want one, right?
Then why would you use the builtin in preference to the parameter SRANDOM?
> Would it be less confusing to refer to it as 'count' (-c) or 'number' (-n)
> instead of length? Maybe it'd be easier to understand that it means byte count
> (not string length) in the default mode and element count in integer mode? I
> guess programming languages often overload 'length' to mean different things
> like that, though, and people get by OK with it
I could see count instead of length, since length is more commonly used 
with the string mode.
>    zwarnnam(name, "argument to -s not an identifier: %s", scalar);
>    zwarnnam(name,"argument to -a not an identifier: %s", arrname);
>
> Seems like it could just create these for you? I'm not aware of any other
> built-ins that take a parameter name that treat non-existence of the parameter
> as an error (except for like typeset). See for example printf and sysread
Copied that pattern straight out of Src/Modules/datetime.c
>    zwarnnam(name, "-r can't be specified with bounds or -i or -a");
>
> I can't think of any reason -r couldn't work with bounds or -a. The latter in
> particular seems useful, though easy enough to achieve with parameter
> expansions i guess. Making it support bounds would let you do things like only
> return random ASCII letters
raw mode isn't something I would think of with processing like bounds.  
It would also restrict the bounds to 0-256.  I would think the -a with 
no -i mode would be more useful for picking out specific characters out 
of a string param of desired characters rather than using the raw ascii 
codes.  I saw raw mode more as passing binary data to a pipe or file.
> Tests and a completion function would be useful
Trying to think how to design tests when the output is different every 
time by design.  And I can't think what a completion function would 
complete.  It's not like it's using long options or enumerated arguments.
> Documentation:
>
>    +Set the length of the returned random data. Defaults to 8.
>
> I feel like it's not immediately clear what 'length' means, due to how it
> behaves with -i. I wasn't sure i was understanding until i just tried it for
> myself. Maybe add something like '(in bytes or, with -i, number of integer
> elements)'?
>
> I also agree with Bart that it makes sense to put -L before -U. Makes sense to
> alphabetise option lists in general imo, though this wouldn't be the first
> built-in in the manual that has them out of order
>
> And i think all of the descriptions should be either indicative ('does x') or
> imperative ('do x'), not a mix of the two. Though again this wouldn't be the
> first to mix them
>
>
> Typos, white space, consistency issues, &c.:
>
> Some typos i noticed:
>
>    +Some High-quality randomness commands, parameters, and functions.
>    *  places them in an outbut buffer twice the size. Returns -1 if it can't.
>    /* Vailues for -U *nd -L */
>    "length must be between 1 and 64 you specified: %d",
>    * Provides for the SRANDOM parameter and returns  a unisgned 32-bit random
>    /* Check for the existence of /dev/urnadom */
>    zwarnnam(name,"-i only makes sense with -s when 1ength is 1");
>    zwarnnam(name,"Upper and Lower bounds cannot be the same.");
>
> git complains about trailing white-space on lines 23, 32, 48, 55
I thought I got all the ones in code.  I wasn't sure enough about YODL 
to remove them in that file.
> Lots of random double-spaces all over
>
> Inconsistent white space around operators, commas, e.g.:
My normal coding style rarely uses unnecessary white space to save line 
width.  It's not surprising I reverted to form a few times and didn't 
catch it on review.
>    zwarnnam(name,"Couldn't get random data");
> but then
>    zwarnnam(name, "-r can't be specified with bounds or -i or -a");
>
> Inconsistent indentation in several places, usually spaces where there should
> be tabs (Ctrl+F eight spaces)
I used editconfig, but when I had to re-indent the line I typed spaces.  
It still seems weird that the dev guide specifies mixing the two.
> Weird error message, not sure what it means for data to fail:
>
>     zwarnnam(name,"Random Data Failed");
>
> Some error messages use sentence case, some don't
>
>
> dana