From: Clinton Bunch <cdb_zsh@zentaur.org>
To: zsh-workers@zsh.org
Subject: Re: [PATCH] zsh/random module
Date: Fri, 4 Nov 2022 01:22:00 -0500 [thread overview]
Message-ID: <1e8ea669-7a25-b321-6024-72dbc43ac023@zentaur.org> (raw)
In-Reply-To: <cc69e1c3-e6d5-44b7-8238-35af27065763@app.fastmail.com>
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
>
next prev parent reply other threads:[~2022-11-04 6:23 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-02 17:13 Clinton Bunch
2022-11-03 17:50 ` Bart Schaefer
2022-11-04 3:17 ` dana
2022-11-04 6:22 ` Clinton Bunch [this message]
2022-11-04 7:27 ` dana
2022-11-04 12:57 ` Clinton Bunch
2022-11-08 0:18 ` [PATCH] zsh/random module [UPDATED] Clinton Bunch
2022-11-18 14:30 ` Clinton Bunch
2022-11-19 6:42 ` Lawrence Velázquez
2022-11-18 16:23 ` Stephane Chazelas
2022-11-18 17:08 ` Clinton Bunch
2022-11-18 18:12 ` Stephane Chazelas
2022-11-18 18:38 ` Clinton Bunch
2022-11-23 19:52 ` Daniel Shahaf
2022-11-24 16:19 ` Stephane Chazelas
2022-11-24 16:30 ` Roman Perepelitsa
2022-11-24 22:39 ` Clinton Bunch
2022-11-25 8:53 ` Stephane Chazelas
2022-11-25 9:40 ` Stephane Chazelas
2022-11-28 16:37 ` further discussion of zsh/random (was [PATCH] zsh/random module [UPDATED]) Clinton Bunch
2022-11-21 1:07 ` [PATCH] zsh/random module [UPDATED] Matthew Martin
2022-11-21 1:59 ` Clinton Bunch
2022-11-21 2:21 ` Matthew Martin
2022-11-21 2:57 ` Clinton Bunch
2022-11-21 3:14 ` Lawrence Velázquez
2022-11-21 4:17 ` Bart Schaefer
2022-11-21 5:05 ` Clinton Bunch
2022-11-22 13:42 ` dana
2022-11-23 19:49 ` Daniel Shahaf
2022-11-22 17:44 ` Oliver Kiddle
2022-11-22 19:48 ` Clinton Bunch
2022-11-23 1:23 ` Matthew Martin
2022-11-23 2:58 ` Clinton Bunch
2022-11-23 4:14 ` Matthew Martin
2022-11-23 13:41 ` Clinton Bunch
2022-11-23 20:33 ` Daniel Shahaf
2022-11-23 21:42 ` dana
2022-11-23 23:54 ` Daniel Shahaf
2022-11-24 0:17 ` Daniel Shahaf
2022-11-24 1:05 ` dana
2022-11-24 13:52 ` Clinton Bunch
2022-11-23 19:46 ` Daniel Shahaf
2022-11-24 2:58 ` Clinton Bunch
2022-11-24 10:07 ` nimaje+zml
2022-11-24 13:19 ` Clinton Bunch
2022-11-24 14:33 ` Clinton Bunch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1e8ea669-7a25-b321-6024-72dbc43ac023@zentaur.org \
--to=cdb_zsh@zentaur.org \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).