#11602: install_scripts should use "$@" instead of $*
------------------------------+---------------------------------------------
Reporter: Stefan | Owner: jason
Type: defect | Status: positive_review
Priority: minor | Milestone: sage-4.7.2
Component: misc | Keywords: install_scripts, hg, command
line
Work_issues: | Upstream: N/A
Reviewer: Leif Leonhardy | Author: John Palmieri
Merged: | Dependencies:
------------------------------+---------------------------------------------
Changes (by newvalueoldvalue):
* status: needs_review => positive_review
* reviewer: => Leif Leonhardy
* author: => John Palmieri
Comment:
Sorry, completely forgot this ticket.
IMHO works as advertised with Sage 4.7.1.rc0, so '''positive review'''.
Only a few minor things:
* Somehow the empty line is removed from the docstring when I type
`install_scripts?`:
{{{
will need write permissions on ``directory``. Note that one good
candidate for ``directory`` is "/usr/local/bin".Running
``install_scripts(directory)`` will be most helpful if
}}}
(Also, `"/usr/local/bin"` should perhaps be double-''back''quoted.)
* You could add yourself to `AUTHORS`. :-)
* I'd quote the `directory` name in the last message. (Also, rerunning
the command may make sense if a newer version of Sage ships more
components, or the user later installs some optional component. Similarly,
a system-wide version might vanish later if it gets uninstalled or the
`PATH` changes.)
* I personally don't like raising `RuntimeError`s for rather "expected"
error conditions; I thought there was a way to at least suppress the
traceback, but I currently don't recall how. (I did get ''"`RuntimeError`
doesn't take keyword arguments"'' when I tried... ;-) ) [[BR]]
If there's no way to suppress the tracebacks, I would catch "expected"
exception(s) and use `sys.stderr.write()`, then proceed with whatever is
appropriate in that situation (e.g. `return`). [[BR]]
(There also could be a ''"Creating script ..."'' message in advance,
since an error in `o.open()` or `o.write()` could otherwise be associated
with ''"Checking that Sage has the command ..."''.)
* An `else:` is superfluous whenever the `if`-branch ends with e.g.
`continue`, but that's partially a matter of taste. (Omitting it here
appears more readable to me, but I don't mind.)
Not that important ''here'', but in general you should pass the ''whole''
environment to `subprocess.call()` with just the variable(s) modified you
have to change, as opposed to passing an "empty" environment with just
these new settings added (like you do now in `have_program()`). Otherwise
settings like e.g. `LD_LIBRARY_PATH` get lost, which might lead to errors
or undesired behaviour.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11602#comment:18>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
