Opened 5 years ago
Closed 5 years ago
#17155 closed enhancement (fixed)
add "sage installed" and "sage pip" commands
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  scripts  Keywords:  
Cc:  tmonteil  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  9810c8c (Commits)  Commit:  9810c8cbf207320cf607c9769b713c29de678112 
Dependencies:  Stopgaps: 
Description
Add two commands to sage:
sage installed
to get the list of installed packagessage pip
to run the Python package manager
Change History (20)
comment:1 Changed 5 years ago by
 Branch set to u/vdelecroix/17155
 Commit set to 4bf23308777288b5a829b18b153367ea0b7a2250
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
Huh  and the pip would be quite useful. But even in the chunk you show,
if [ "$1" = 'i' ]; then shift # If there are no further arguments, simply list all installed
This should even be documented somewhere, but maybe it hasn't been advertised well lately? Not that it would be horrible to add this alias, though I don't know if some syntax warriors will complain because you allow installed
while installed
is the current vogue...
comment:3 Changed 5 years ago by
Hello,
Thanks for having a look.
I do not complain that sage i
has not been advertised. But I do that it is documented nowhere (neither in src/bin/sage
nor in src/bin/sagepkg
)!
Currently we have the options optional
, standard
and experimental
. I am just adding an additional installed
. We can be smarter and more coherent by allowing multiple arguments and force to use the i
option like in
$ sage i installed optional standard ... would list all installed package that are either ... ... optional or standard ... $ sage i notinstalled ... would list all non installed packages ...
I am in favour of a more coherent scheme of options, but then we should forbid the older syntax (and instead write a deprecation message that explains the new syntax).
And I definitely do not understand why we do have two scripts (src/bin/sagespkg
and src/bin/sagelistpackages
) for dealing with packages. Morever the current state is that sagespkg
is used to list the installed packages while sagelistpackages
is the one to list the optional packages...
EDIT: and I just noticed that the sage i
starts with a useless line 'Currently installed packages:'. It is especially bad if you want to use it in other scripts.
Vincent
comment:4 Changed 5 years ago by
And I definitely do not understand why we do have two scripts
Alas, I can only point things out when it comes to these scripts  I am reluctant to tread on such longstanding traditions. I do hope some others come to look at this, though, since at the very least it is not bad to add installed
.
comment:5 followup: ↓ 8 Changed 5 years ago by
 Status changed from needs_review to needs_work
Use spaces, not TABs for indentation.
Instead of [ ! $(sage installed  grep c '^pip') eq 1 ]
,
I would go with [ x "$SAGE_LOCAL/bin/pip" ]
And why not replace echo "Pip is not installed. Run \"sage i pip\" to install it."
by sage i pip  exit $?
?
comment:6 Changed 5 years ago by
 Commit changed from 4bf23308777288b5a829b18b153367ea0b7a2250 to 6ae094b15c7e966a38ca92a280b7974b83c530c7
Branch pushed to git repo; I updated commit sha1. New commits:
6ae094b  trac #17155: reviewer comments

comment:7 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:8 in reply to: ↑ 5 Changed 5 years ago by
Replying to jdemeyer:
Use spaces, not TABs for indentation.
Instead of
[ ! $(sage installed  grep c '^pip') eq 1 ]
, I would go with[ x "$SAGE_LOCAL/bin/pip" ]
done. It is much simpler, thanks.
And why not replace
echo "Pip is not installed. Run \"sage i pip\" to install it."
bysage i pip  exit $?
?
I do not like to do things in a script that users did not ask for.
Vincent
comment:9 Changed 5 years ago by
 Commit changed from 6ae094b15c7e966a38ca92a280b7974b83c530c7 to 0ded194dd551dc9a29b1991e0b782e53faa770da
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0ded194  trac #17155: reviewer comments

comment:10 Changed 5 years ago by
 Status changed from needs_review to needs_work
If you deprecate something, it should still work (i.e. don't remove the old code, just add the echo
statements).
Also, please use echo >&2
for these warnings.
comment:11 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
In help
, you write installed
while only installed
works. I think you should stick to the de facto convention that both single and double dashes should work.
comment:12 Changed 5 years ago by
 Commit changed from 0ded194dd551dc9a29b1991e0b782e53faa770da to 31c2fec6204031e9882d278eb2ecd79bbdf74569
Branch pushed to git repo; I updated commit sha1. New commits:
31c2fec  trac #17155: nicer deprecation

comment:13 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 5 years ago by
 Status changed from needs_review to needs_work
In sagelistpackages
:
"standard
>"standard"
except A, B
>except A as B
 The
except
clause should abort the program otherwise one getsTraceback (most recent call last): File "/usr/local/src/sageconfig/src/bin/sagelistpackages", line 80, in <module> available_version = dict([ split_pkgname(pkg) for pkg in get_remote_package_list(category)]) TypeError: 'int' object is not iterable
comment:15 Changed 5 years ago by
 Commit changed from 31c2fec6204031e9882d278eb2ecd79bbdf74569 to 9810c8cbf207320cf607c9769b713c29de678112
Branch pushed to git repo; I updated commit sha1. New commits:
9810c8c  trac #17155: reviewer comments

comment:16 Changed 5 years ago by
 Status changed from needs_work to needs_review
Thanks for your comments. I did all the modifications in my last commit.
comment:17 Changed 5 years ago by
ping!
comment:18 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:19 Changed 5 years ago by
Thanks.
comment:20 Changed 5 years ago by
 Branch changed from u/vdelecroix/17155 to 9810c8cbf207320cf607c9769b713c29de678112
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #17155: add "sage installed" and "sage pip" commands