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:

Status badges

Description (last modified by jdemeyer)

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)

trac_11993-fix_sage_--version.scripts.patch (892 bytes) - added by leif 10 years ago.
SCRIPTS repo. Based on Sage 4.7.2.
trac_11993-fix_sage_--version.scripts.v2.patch (673 bytes) - added by jdemeyer 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by leif

  • Cc jhpalmieri added
  • Description modified (diff)
  • Status changed from new to needs_review

While we're at it, one for you to review...

comment:2 Changed 10 years ago by leif

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 leif

  • Keywords sage-sage added

comment:4 follow-up: Changed 10 years ago by jhpalmieri

  • 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.

Changed 10 years ago by leif

SCRIPTS repo. Based on Sage 4.7.2.

comment:5 in reply to: ↑ 4 Changed 10 years ago by leif

  • 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 jhpalmieri

  • 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: Changed 10 years ago by jdemeyer

  • 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

  1. you do it on this ticket
  2. 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: Changed 10 years ago by jhpalmieri

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 jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to positive_review

comment:10 in reply to: ↑ 8 Changed 10 years ago by jdemeyer

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.

comment:11 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.