Re: [PATCH] zsh/random module

[dana <dana@dana.is>]

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?

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

  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

  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

Tests and a completion function would be useful


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

Lots of random double-spaces all over

Inconsistent white space around operators, commas, e.g.:

  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)

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