Opened 9 years ago
Closed 9 years ago
#13113 closed defect (fixed)
sage -pkg can't handle its own options
Reported by: | kini | Owned by: | leif |
---|---|---|---|
Priority: | major | Milestone: | sage-5.2 |
Component: | scripts | Keywords: | sd41 sage-pkg |
Cc: | jhpalmieri | Merged in: | sage-5.2.beta0 |
Authors: | Keshav Kini | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
sage -pkg -n dir1
fails because the main sage command line handler actually turns this into basically sage-pkg -n && sage-pkg dir1
, which is stupid since sage-pkg
is already using OptParse
to parse arguments and is perfectly capable of handling all of its command line arguments at once. These patches make it do so.
Attachments (2)
Change History (21)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
Oops, I lied - another difference is that I moved the message about what package we're creating from $SAGE_ROOT/spkg/bin/sage
to $SAGE_LOCAL/bin/sage-pkg
, and also I removed line 55 for obvious reasons.
The first of these fixes this stupid situation:
[1] fs-boone@zhenghe /opt/sage/local/bin $ sage -pkg --help Creating Sage package --help Usage: sage-pkg [options] Options: -h, --help show this help message and exit -n, --no-compress, --no_compress, --nocompress don't compress the spkg file
(Note the second line.)
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Keywords sd41 sage-pkg added
comment:5 Changed 9 years ago by
- Cc jhpalmieri added
CCing jhpalmieri since he might be interested... maybe... ? :)
comment:6 Changed 9 years ago by
You don't need leading spaces on lines 74-75 or 77. I think the same is true for selected lines in the range 102-126. Otherwise, the patches look good in principle. I haven't actually tested them, of course...
comment:7 Changed 9 years ago by
Whoops, you're right, of course. New patch in a sec
comment:8 Changed 9 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_review
Looks good to me, and works the way it should.
comment:9 Changed 9 years ago by
Awesome, thanks!
comment:10 Changed 9 years ago by
- Status changed from positive_review to needs_work
Wait, the root repo patch needs work - I should handle --pkg_nc properly.
comment:11 Changed 9 years ago by
- Status changed from needs_work to needs_review
Also replicated the message from --pkg_nc
in sage-pkg
(new line 53 in the scripts patch).
comment:12 Changed 9 years ago by
- Status changed from needs_review to positive_review
Still looks good.
comment:13 Changed 9 years ago by
- Status changed from positive_review to needs_work
What's the point of the two
if [ $? -ne 0 ]; then exit 1 fi
blocks?
They could just be removed, right?
comment:14 Changed 9 years ago by
Well, they were there before, so I just left them there. If you think removing them is that important, I'll update the patches...
comment:16 Changed 9 years ago by
I still think this looks good. Jeroen?
comment:17 Changed 9 years ago by
- Status changed from needs_review to positive_review
Looks good to me, and I like your commit message :-)
comment:18 Changed 9 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:19 Changed 9 years ago by
- Merged in set to sage-5.2.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
The second patch is kind of hard to read - all I did between the
for
statement I added and theif __name__ == '__main__'
at the bottom was to indent everything by four spaces.