Re: Review of new man page of sensible-editor
This really needs a whole new proposed version, but that isn't
attached yet.
RL wrote:
>> This is a bit obscure, since no $0 has been mentioned. If I'm fluent
>> in shell and want to know exactly how sensible-editor is implemented,
>> can't I just read it?
>
> agree - the whole section seems like an implementation detail that is
> not needed?
And in fact it isn't code that's used within the sensible-editor
script. Nor is it functionality that's defined in environ(7).
>> .SH "STANDARD"
>> -Documentation of behavior of sensible-utils under a debian system is available under
>> -section 11.4 of debian-policy usually installed under
>> +Documentation for the behavior of sensible-utils under a Debian system is available under
>> +section 11.4 of Debian-Policy, usually installed under
>> /usr/share/doc/debian-policy (you might need to install debian-policy)
>>
>> It's a bit odd to expect users to install a package that's aimed at
>> Debian developers when they could just read it online at
>> https://www.debian.org/doc/debian-policy/
>> but I suppose man pages tend to prefer pointers to local resources.
>
> someone (with debian-policy installed) might be trying to launch an editor to fix their networking
> setup perhaps? (which doesnt mean it couldnt reference both locations)
(If my networking's broken, I'm also likely to have trouble installing
new documentation packages!)
> Comments on the revised version - from my perspective as a user. Can
> make a patch if helpful.
>
>> .\" -*- nroff -*-
>> .TH SENSIBLE-EDITOR 1 "14 Nov 2018" "Debian"
>
> Date needs updating?
>
>> .SH NAME
>> sensible-editor \- sensible editing
>
> "editing" seems unhelpful as it is not an editor - just a way to launch
> an editor. "launch an editor" migth be clearer?
It doesn't exactly claim that it *does* the editing; it's a wrapper
script that *enables* editing, in a "sensible" manner. But maybe it
could be more informative!
It should definitely keep the why-the-name hint, but I'd be happier if
it indicated that the thing that's sensible isn't, for instance, the
keybindings, it's the process of choosing what editor to invoke. So
maybe
sensible-editor \- launch sensibly chosen text editor
>>.SH SYNOPSIS
>>.BR sensible-editor " [OPTIONS...]"
>
> spaces look odd here? (wouldnt this tokenize the opening and closing
> quotes differently? - i admit i didnt check)
>
>
>> .br
>
> not needed?
>
>> .SH DESCRIPTION
>> .BR sensible-editor " makes sensible decisions on which editor to call.
>
> is a single " correct?
These all seem to render correctly for "man -l -- $FILE".
>> Programs in Debian can use this script
>> as their default editor.
>
> "as" and "their" seem inacccurate - programmes can use the scipt to
> launch the user's default editor
And when I edit this email, it isn't the file /bin/nano that does the
editing, either; that binary just loads an application into my PC's
memory, calling on various system libraries in the process! But the
wording here allows for the interpretation that the programs can
invoke this script *as if* sensible-editor was itself a text editor.
>> environment variable exists, execute
>
> "execute" is unclear to me, esp when applied to an environment variable - "run" might be clearer?
It's clear enough as long as you interpret "VARIABLE" as the variable
name and "$VARIABLE" as the *expansion* of that variable, so that
"execute $VARIABLE" means "execute the command named by the variable".
If you don't interpret it that way, substituting "run" won't make that
part any clearer. This would be better solved by starting with a
general summary - something like "sensible-editor starts by looking
for the name of a text editor command in several environment
variables. If it doesn't find any then it uses the default 'editor'
command defined by the alternatives system (or if that also isn't
available, some other fallback option). Specifically..."
(I dislike the way it uses vi as its ultimate fallback, which can
leave naive newbies desperately trying to find the key sequence that
will close the file without sabotaging it. In my opinion if the
alternatives system says there's no appropriate program available then
sensible-editor, -pager and -browser should all fall back on running
/bin/more, which unlike vi is guaranteed to be present.)
>> .B $VISUAL [OPTIONS...];
>
> should his be .BR rather than .B? (with differnt choice of spaces
> perhaps? - would want consistency with the synopsis line)
>
>> if this fails, it continues to the next step
>> .IP \n+[step]
>> if the
>
> capitalise as "If" at the start of each step? (applie throughout)
It depends whether we're treating them as sentences or as effectively
parts of one big sentence. It also depends whether we're following a
styleguide that likes capitalisation after a colon.
>> .B EDITOR
>> environment variable exists, execute
>
> "exists" is a bit unclear, bash distiguishes between "set" and "empty" variables: is
> "is non-empty" better here?
It's actually a sh script (which makes testing whether a variable
*exists* into an annoying task), but yes, it uses [ -n "$VARIABLE" ].
Of course, if sensible-editor did accept null variables as passing the
test, it would end up trying to execute the empty string, which seems
a good reason to take it for granted that it doesn't accept them.
>> .B $EDITOR [OPTIONS...];
>> if this fails, it continues to the next step
>
> what does "it" refer to here? (replace with "continue" or reword )
This list was introduced in terms of sensible-editor doing a series of
things, so "it" is sensible-editor. Mind you, it would make just as
much sense if you misinterepreted it as saying something like "the
decision-making logic continues to the next step".
>> environment variable exists, execute
>
> as above, "exists" and "execute" could be edited
>
>> .B $SENSIBLE_EDITOR [OPTIONS...];
>> if this fails, it continues to next step
>
> what does "fails" mean - somehing exiting non-zero or is this still
> about variables being non-empty?
It sounds as if it means "returns an exit code indicating failure",
but in fact (as it goes on to explain later) sensible-editor doesn't
care if your chosen editor is something like /bin/false that runs and
returns 1; it only proceeds to the next step if it returned 126 or 127
(cannot-execute or command-not-found).
>> .IP \n+[step]
>> source the contents of file
>> .I ~/.selected_editor
>
> "of the file"
>
> but "source" is unclear as there is no explanation of what language/syntax.
Well, you're aware of sourcing as a thing that shells implement via
builtins with names like "source" or ".", right? It uses the one in
the language it's running. I'm *happy* it's explaining what it's
trying to do instead of just reciting the code that implements it!
But it might be even better if it just said that it checks whether a
default editor has been defined via select-editor, and delegated the
technical details of how that works to select-editor(1).
Come to think of it, when sensible-editor(1) describes SELECTED_EDITOR
as an environment variable, that technically isn't true - it gets the
value from a file, not the environment.
>> and, if the
>> .B SELECTED_EDITOR
>> environment variable exists, execute
>
> "now exists"? (are you saying that .sensible-editor is meant to set this
> variable?)
Yes, saying "now exists" would definitely make this clearer.
> "run" not "execute" as above
>
>> .IP \n+[step]
>> run the
>> .B editor [OPTIONS...]
>> command;
>> if this fails, it continues to the next step
>> command
>
> "command" is superflous here? (and inconsistent with the other steps)
I saw that one then forgot it again, thanks for catching it.
>> .IP \n+[step] run
>> run the
>> .B nano [OPTIONS...]
>> command;
>> if this fails, it continues to the next step
>> .IP \n+[step]
>> run the
>> .B nano-tiny [OPTIONS...]
>> command;
>> if this fails, it continues to the next step
>> .IP \n+[step]
>> finally run the
>> .B vi [OPTIONS...]
>> command;
>> if this fails, it errors out.
>
> seems long-winded: "the folllowing caommands are tried in order: nano,
> nano-tiny, etc"
Yes. See below for prototype rewrite throwing out most of this.
>> .SH "NOTES"
>> This script executes environment variables as specified in
>> .BR environ (7):
>
> not sure anyone is helped by this ref to environ(7). sugggest this whole
> note is an implementation detail that should be deleted
Or just moved to CONFORMING TO (which for some reason is here given
the non-standard heading STANDARD).
>> .PP
>> An exit status of 126 (command is found but is not executable) or 127 (given command is not found within your .B $PATH)
>> are considered as indicating the failure of a command, for the purposes of sensible-editor.
>
> Maybe .SH EXIT STATUS instead of .PP?
That would make it sound as if it was talking about sensible-editor
giving an exit code of 127, which would be bizarre.
> "given" and "your" seem superfluous after the 127?
>
> the last line is really passive voice and hard to follow. (is this still
> about finding an editor or what happens if that editor exits non-zero?)
De-contorting it, what it's trying to do is define what the above
pseudocode means by "fail". All of this would be better explained
once at the top of DESCRIPTION. Something like
sensible-editor makes sensible decisions on which editor to call.
Programs in Debian can invoke this script to get their default editor.
sensible-editor starts by looking for the name of a text editor
command in a series of variables, using the first one that works. If
it doesn't find any then it uses the default 'editor' command defined
by the alternatives system (or if that also isn't available, some
other fallback option).
For environment variables, any non-null string will be accepted, and
may include extra whitespace-separated parameters such as a
"--verbose" flag. Once sensible-editor has a candidate commandline,
it will try to run it (passing on the arguments it was given as the
files to be edited); if this fails because the command was not found
or couldn't be executed, it tries the next option.
The specific steps sensible-editor goes through are
* it checks the environment variables VISUAL. EDITOR, or
SENSIBLE_EDITOR
* if none of those work, it tries to read ~/.selected_editor and use
the variable SELECTED_EDITOR defined there (see select-editor(1))
* if that doesn't work, it tries the executable names 'editor',
'nano', 'nano-tiny', or as a last resort 'vi'.
>> .SH "SEE ALSO"
>> .BR environ (7)
>> for documentation of the
>> .B EDITOR,
>> .B VISUAL
>> variables
>
> "and" not ","?
>
>> for changing a user's default editor.
>
> "for how to change"?
>
>> for default system wide editor.
>
> "for the"
Yes, I think I must have fallen asleep for this bit.
>> .SH BUGS
>> This command is protected against the trivial fork bomb when a user
>> sets
>
> "a trivial fork bomb"?
>
> but "fork bomb" is unhelpful jargon - "This command is clever enough not
> to create an infinite loop if
> .B EDITOR=sensible-editor
> but other loops can happen if <is there an example?>
>
> "?
It must mean that it won't protect you if you're foolish enough to
make EDITOR point at my-favourite-editor, which is a shell wrapper
invoking sensible-editor. Oh yes, the bullseye version has some
whichcraft at the top of the script, that'll need changing for
bookworm!
>> but wider loops are still possible.
>
> how does one measure the "width" of a "loop"?
"Length" might make more sense! But it might as well say explicitly
"but this will only protect against immediate loops, not anything more
complicated".
>> .SH "STANDARD"
>
> odd section title - suggest mergin into "see other" section
Either that or it should *also* mention the standard of respecting
EDITOR and VISUAL as mentioned in environ(7).
--
JBR with qualifications in linguistics, experience as a Debian
sysadmin, and probably no clue about this particular package
Reply to: