Opened 10 years ago
Closed 10 years ago
#11993 closed defect (fixed)
Fix output of `sage --version`
Reported by: | leif | Owned by: | leif |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.8 |
Component: | scripts | Keywords: | banner sage-sage |
Cc: | jhpalmieri | Merged in: | sage-4.8.alpha1 |
Authors: | Leif Leonhardy | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The one who was aiming at the Useless Use of cat
Award ![1] unfortunately did some wrong escaping, so we currently get for example:
$ ./sage --version
| Sage Version 4.7.2.alpha3-pre7-without-examples, Release Date: 2011-09-28 |
* Warning: this is a prerelease version, and it may be unstable. *
Also, I don't think the second line should be printed, hence the new version also case-sensitively "greps" for Version
, such that we get:
$ ./sage --version
Sage Version 4.7.2.alpha3-pre7-without-examples, Release Date: 2011-09-28
![1] http://partmaps.org/era/unix/award.html
See also The Tragedy of Bash.
Apply trac_11993-fix_sage_--version.scripts.v2.patch to the Sage scripts repository.
Attachments (2)
Change History (13)
comment:1 Changed 10 years ago by
- Cc jhpalmieri added
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
In case someone should insist on the second line also getting printed, here we go:
sed -n -e '/[Vv]ersion/s/\(^[ |*]\+\)\|\([ |*]\+$\)//gp' ...
comment:3 Changed 10 years ago by
- Keywords sage-sage added
comment:4 follow-up: ↓ 5 Changed 10 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to needs_work
On sage.math:
$ sage -v Sage Version 4.7.2.alpha4, Release Date: 2011-10-11 $
On OS X:
$ sage -v $
Same thing happens on Solaris and OpenSolaris. You must be using some GNUisms in your invocation of sed.
comment:5 in reply to: ↑ 4 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to jhpalmieri:
You must be using some GNUisms in your invocation of sed.
What a terrible failure... 8/
New patch, same place.
comment:6 Changed 10 years ago by
- Status changed from needs_review to positive_review
As far as I can tell, you don't need the -e
argument for sed, but it doesn't seem to hurt anything either.
I'm also fine with the decision not to print the second line ("* Warning...").
comment:7 follow-up: ↓ 8 Changed 10 years ago by
- Status changed from positive_review to needs_work
I disagree with makeing here the change
if [ "$1" = '-v' -o "$1" = '-version' -o "$1" = '--version' ]; then
to
case "$1" in -v|-version|--version)
I don't disagree with the change itself, just with the fact that
- you do it on this ticket
- only for this command line option
If you want to make this change, please do it on a different ticket and then for all command-line options.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 10 years ago by
Replying to jdemeyer:
I disagree with makeing here the change...
I don't have a strong feeling about this, but I think your criticism would be more valid if the script sage-sage were a model of consistency and good style; then altering the style of one part would be a bad idea. But I don't think it is such a model.
If you want to make this change, please do it on a different ticket and then for all command-line options.
I think that we should work on #21 (using a real argument parser and starting to enforce double-hyphens for long options). That ticket needs a lot of work — I'm not sure I like my original approach there, so I may try to rewrite it some day — but rather than patching style issues in sage-sage, it would be better to put the effort elsewhere.
comment:9 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
comment:10 in reply to: ↑ 8 Changed 10 years ago by
Replying to jhpalmieri:
I don't have a strong feeling about this, but I think your criticism would be more valid if the script sage-sage were a model of consistency and good style;
Actually, I do think it has a consistent bad style.
At least let's not make it less consistent.
Here is a v2 patch, changing only the command itself, not the option parsing.
Changed 10 years ago by
comment:11 Changed 10 years ago by
- Merged in set to sage-4.8.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
While we're at it, one for you to review...