Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: [PATCH] zsh/random module



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






Messages sorted by: Reverse Date, Date, Thread, Author