#11142 closed enhancement (fixed)
clean up sage/misc/hg.py
Reported by: | jhpalmieri | Owned by: | jason |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | misc | Keywords: | sd31 |
Cc: | kini | Merged in: | sage-4.7.2.alpha3 |
Authors: | John Palmieri | Reviewers: | Karl-Dieter Crisman, Keshav Kini |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10594 | Stopgaps: |
Description (last modified by )
The attached patch cleans up hg.py in a few ways:
- it adds a few doctests, although many of them are marked "not tested", since otherwise it would try to import or export non-existent patches, etc.
- the function "pager" is rewritten, to use the Mercurial pager extension. This way, any highlighting is preserved when you use "hg_sage.diff()" or "hg_sage.log()" with the color extension enabled.
- the function "color" was added, to disable the Mercurial color extension in the notebook. Otherwise it produces some bad output: try putting
[extensions] color =
in your .hgrc file, modify some files in the Sage library, and type "hg_sage.status()" in the command line vs. the notebook.
Apply
to the Sage library.
Attachments (8)
Change History (43)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
comment:3 follow-up: ↓ 5 Changed 11 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
Yup, you're right that it depends on adding the pager stuff to work (though not to apply). Overall this looks good.
Okay, I also have a question. Why the debug=False
? You did a nice job with this and the pager, but shouldn't we print out the commands? Or is that just annoying to see? Just wondering.
'Needs work' for adding the very important options in the documentation to revert, rollback, and import. If we are going to update doc at all, this should happen.
comment:4 Changed 11 years ago by
- Keywords sd31 added
comment:5 in reply to: ↑ 3 Changed 11 years ago by
- Status changed from needs_work to needs_review
Replying to kcrisman:
Okay, I also have a question. Why the
debug=False
? You did a nice job with this and the pager, but shouldn't we print out the commands? Or is that just annoying to see? Just wondering.
I thought it was little annoying to see. Typical output might be
cd "/Applications/sage/devel/sage" && hg diff --config pager.pager="LESS='FSRX' less"
But I've changed it to True.
'Needs work' for adding the very important options in the documentation to revert, rollback, and import. If we are going to update doc at all, this should happen.
Okay, I've taken care of this.
Now, changing the pager as I've done changes the current behavior in several ways (from the command line, not the notebook: the notebook should behave the same way):
- (the option F says to quit the pager if everything fits on one page; this doesn't change the current behavior, but see option X below.)
- the option S says to truncate long lines rather than wrap them. Currently we wrap them, as in
changeset: 15816:11586fb87f83 user: John Cremona <john.cremona@gmail.com> and Robert Bradshaw <robertwb@math.washington.ed u> date: Fri Apr 22 15:50:26 2011 +0100 summary: Trac #11220: #11220: implement CM j-invariants and orders for quadratic fields
Note lines 2-3. With the S option, line 3 would be omitted. Opinions? - the option R says to preserve any color information in the output, so output from hg_sage.diff() would be colorized nicely. This is one of the points of this ticket, so I definitely want to keep this.
- the option X says to not clear the screen when you quit the pager. I happen to like this. However, if this is off but option F is on, then any output containing less than a screen displays briefly, and then the screen gets cleared, so it's gone. This is not Sage-specific: it reproduces the behavior when you run "hg diff" from the shell, with
pager = LESS='FSR' less
in the[pager]
section of your .hgrc file.
My suggestion: we use either "FSRX" or "FRX", and probably the second of these is better. That's what the latest version of the patch uses.
Another question: should we try to have a way to configure the pager options? We can accept keyword arguments into the various hg commands and then pass them to the pager command, or we can do it like the way latex options are set. Is it worth it?
comment:6 follow-up: ↓ 7 Changed 11 years ago by
Personally I don't like the F
option (unpredictable behavior), X
option (if I wanted to read the last screen of the paged stuff, I wouldn't have quit less
), or S
option (information loss), but that's just me.
ipython's native pager (for help lookups, etc.) acts most like FR
, fwiw.
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Replying to kini:
Personally I don't like the
F
option (unpredictable behavior),X
option (if I wanted to read the last screen of the paged stuff, I wouldn't have quitless
), orS
option (information loss), but that's just me.
If I look at a docstring (which displays with a pager), sometimes I want to refer to the docstring when typing the next command, and then it's really frustrating that it went away when I typed "q" to quit the pager program. The same can happen with hg commands, although not as frequently, at least for me. However, it can also be nice to restore what was there before I started reading the docstring. We could add an option for any hg command which uses the pager: by default, it will open with "R", but with the option, it will open with "FRX".
comment:8 Changed 11 years ago by
Here are two more patches. One replaces the old patch, and this just changes the options for "less" to "R" instead of "FRX". The second adds lots of methods for Mercurial queues, and it brings doctest coverage up to 97% -- I can't figure out how to write a helpful and/or meaningful doctest for get_remote_file
. So the second is more ambitious. If you think it belongs on another ticket, we can just work with the first version.
comment:9 Changed 11 years ago by
- Dependencies set to #10594
- Description modified (diff)
comment:10 follow-up: ↓ 12 Changed 11 years ago by
Hey John, I really like the second patch, so much good (if tedious) work on a lot of things. Have not yet tested it, and can't tonight, though it applies fine.
- Does
hg_sage.revert()
really just revert all files? I always neededoptions="--all"
, but maybe I don't any more. - What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)
- I haven't tried
hg_sage.root
but I assume this was enabled when the root directory finally got a repository?
comment:11 Changed 11 years ago by
comment:12 in reply to: ↑ 10 ; follow-up: ↓ 13 Changed 11 years ago by
- Does
hg_sage.revert()
really just revert all files? I always neededoptions="--all"
, but maybe I don't any more.
Yeah, this does not work.
sage: hg_sage.revert() cd "/Users/.../sage-4.7.1/devel/sage" && hg revert abort: no files or directories specified; use --all to revert the whole repo
This is even after the patch and hg upgrade. We definitely need to have documentation for this. Ideally, we'd even catch it in hg_sage.revert
itself. It takes a while to figure out you need options="--all"
as a newbie.
Or we could change the behavior so that it does this.
- What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)
Ok, Keshav, as long as you vouch for this. Changing my hgrc didn't seem to do anything (the desired behavior), but maybe the repository already had hg initialized with mq or something...
- I haven't tried
hg_sage.root
but I assume this was enabled when the root directory finally got a repository?
Yes. Cool! Now I can edit the README more effectively :)
Current patch (with R, not FRX) does indeed clear the screen after a short view in the pager is quit. I thought this was not the current behavior, but apparently I get that with my 4.7 and 4.4.4 installs as well, so I'm a bit confused.
Otherwise we just need to test a lot of this stuff, but it looks good.
comment:13 in reply to: ↑ 12 ; follow-up: ↓ 18 Changed 11 years ago by
Replying to kcrisman:
- Does
hg_sage.revert()
really just revert all files? I always neededoptions="--all"
, but maybe I don't any more.Yeah, this does not work.
I actually think it's a bit dangerous to be able to revert without specifying "--all", so I think we should just fix the documentation, not change the behavior. I'll do this soon.
- What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)
Ok, Keshav, as long as you vouch for this. Changing my hgrc didn't seem to do anything (the desired behavior), but maybe the repository already had hg initialized with mq or something...
As Keshav says, the latest Mercurial spkg turns on queues in SAGE_ROOT/local/etc/mercurial/hgrc
, so this is not an issue. (This is why #10594 is a prerequisite for this ticket.)
- I haven't tried
hg_sage.root
but I assume this was enabled when the root directory finally got a repository?Yes. Cool! Now I can edit the README more effectively :)
Right, ticket #9433, in particular trac_9433-sage-repo.2.patch, added this.
comment:14 Changed 11 years ago by
I actually think it's a bit dangerous to be able to revert without specifying "--all", so I think we should just fix the documentation, not change the behavior. I'll do this soon.
Can you at that time also make sure to document the (very useful) --no-commit
option for import_patch
?
Still testing...
comment:15 Changed 11 years ago by
Here's a BIG problem, but very easy to fix:
sage: hg_sage.qpop() cd "/Users/.../sage-4.7.1/devel/sage" && hg qpush applying trac_11142-sage-hg.patch
Oops! That's the only one like that I found in the queues stuff, though.
comment:16 Changed 11 years ago by
- Status changed from needs_review to needs_work
- Work issues set to pop\neq push and a couple minor documentation things
comment:17 Changed 11 years ago by
But otherwise debug
and verbose
function properly, I tried lots of commands with various options, and it seems to work exactly the same as hg *
from my command line (which uses the HG in Sage, of course). Correctly accesses internet. Passes tests. So I think that the issues in 'work issues' are probably all that needs to be fixed.
Nice work! Now that I know how to use queues, I might end up being even more productive having them in the command line Sage. Thanks.
comment:18 in reply to: ↑ 13 Changed 11 years ago by
- Status changed from needs_work to needs_review
Re the qpush/qpop thing: I'm just like trying to keep people on their toes.
Re the documentation: I wonder if Mercurial itself used to revert all files when there were no arguments given: the rest of the docstring seems to have been taken from the help message printed by "hg help revert". I've now replaced it with the current version of that message, edited slightly to use Sage hg_sage....
syntax instead of hg ...
shell syntax.
There is more work to be done, perhaps: commands like "hg status" can take many options when executed from the shell, but the Sage implementation doesn't include any of them. If someone feels like opening up a follow-up ticket to implement things like that, they could. Also, it wouldn't hurt to make sure that our docstrings are in line with Mercurial's current docstrings. Since using Mercurial from the shell is pleasant enough, I would give these tasks a pretty low priority, as long our our docstrings don't say things which are completely wrong, like your catch with revert
.
comment:19 Changed 11 years ago by
- Description modified (diff)
- Work issues pop\neq push and a couple minor documentation things deleted
Oops, forgot "--no-commit". Now it's there, too.
comment:20 Changed 11 years ago by
There are a bunch of things that didn't seem to look so good in the documentation. Then I went ahead and made various other minor changes in a reviewer patch. You can pick and choose, or just say positive review. I tried to actually build the changes, but my Sage is funny sometimes and doesn't recognize changed files for the docbuild.
The only ones I would insist on are
- To see the changes in this file since revision 10000: + To see the changes in this file since revision 10000::
and the stuff about emailing William with a bundle, which is totally outdated in several ways.
I also wonder if you wanted to use ::
for the various options for the hg commands. It looks nice, but in other places (like plot documentation) we just use the Sphinx subitem syntax, I think, so maybe we shouldn't make those look like examples. I am okay with either one, just wanted to ask about this.
Positive review on the updates and patch, though. I'm already using it!
comment:21 Changed 11 years ago by
I'm sorry about the name. It's for 11142, of course.
comment:22 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
The changes look good to me. Thanks for cleaning that up.
The nice thing about the "::" for the option lists is that it uses a fixed-width font, so the hyphens line up, etc. Speaking of which, I'm putting up a new version of my patch which just adds a missing space in one of these option lists:
-
sage/misc/hg.py
diff --git a/sage/misc/hg.py b/sage/misc/hg.py
a b class HG: 527 527 %b basename of the exporting repository 528 528 %h short-form changeset hash (12 bytes of hexadecimal) 529 529 %n zero-padded sequence number, starting at 1 530 %r zero-padded changeset revision number530 %r zero-padded changeset revision number 531 531 532 532 - ``text`` - boolean (default False). Setting this to be True 533 533 has the same effect as passing the "-a" option below.
comment:23 Changed 11 years ago by
- Status changed from positive_review to needs_work
I get several doctest failures of the form
sage -t -force_lib devel/sage/sage/misc/hg.py ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage-main/sage/misc/hg.py", line 783: sage: hg_sage.rename('sage/misc/hg.py', 'sage/misc/hgnew.py', options='--dry-run') Expected: Moving sage/misc/hg.py --> sage/misc/hgnew.py cd ... && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py" Got: Moving sage/misc/hg.py --> sage/misc/hgnew.py <BLANKLINE> WARNING: Make sure to create a ~/.hgrc file: ---------------------------------------------------------------------- [ui] username = William Stein <wstein@gmail.com> ---------------------------------------------------------------------- <BLANKLINE> <BLANKLINE> cd "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage" && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py" **********************************************************************
I believe the doctests should be written in such a way that they do not require a .hgrc
file. Also, are the messages
Adding file module_list.pyc cd "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage" && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py"
and
WARNING: Make sure to create a ~/.hgrc file: ---------------------------------------------------------------------- [ui] username = William Stein <wstein@gmail.com> ----------------------------------------------------------------------
written to stdout
or stderr
? The second one certainly should be written to stderr
. If the first is written to stdout
, the problem can be solved by capturing only stdout
.
comment:24 Changed 11 years ago by
The warning about the .hgrc
file is printed by the _warning
method, not by Mercurial itself. What do you think about not printing that warning if doctesting? I'm considering this patch:
-
sage/misc/hg.py
diff --git a/sage/misc/hg.py b/sage/misc/hg.py
a b 336 336 337 337 sage: hg_sage._warning() # random 338 338 """ 339 if not os.path.exists(os.path.join(os.environ['HOME'], '.hgrc')): 339 from sage.plot.plot import DOCTEST_MODE 340 if not os.path.exists(os.environ['HOME'] + '/.hgrc') and not DOCTEST_MODE: 340 341 print "\nWARNING:" 341 342 print "Make sure to create a ~/.hgrc file:" 342 343 print "-"*70
With this, all tests pass even if I don't have a .hgrc
file.
comment:25 Changed 11 years ago by
Good idea. But why on earth is DOCTEST_MODE
in sage.plot.plot
?
comment:26 Changed 11 years ago by
Er, also, why change the os.path.join
to string concatenation?
comment:27 Changed 11 years ago by
kini: Sorry, that was a mistake.
comment:28 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Okay, here is a new patch.
comment:29 Changed 11 years ago by
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Keshav Kini
- Status changed from needs_review to positive_review
All doctests pass. Looks good! Positive review from me.
comment:30 Changed 11 years ago by
- Description modified (diff)
It would certainly be better to reupload a patch with a misspelled name, i.e., ticket number.
comment:31 follow-up: ↓ 32 Changed 11 years ago by
I don't think it matters - the filename of the patch isn't stored anywhere in the Mercurial history and the commit message itself is accurate.
comment:32 in reply to: ↑ 31 Changed 11 years ago by
Replying to kini:
I don't think it matters - the filename of the patch isn't stored anywhere in the Mercurial history and the commit message itself is accurate.
Well, there's sometimes a small period of time between when a patch gets uploaded and when it gets merged.
comment:33 Changed 11 years ago by
- Description modified (diff)
If it helps you with release managing, why not :)
comment:34 Changed 11 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:35 Changed 11 years ago by
See #11898 for a problem probably caused by this.
This looks pretty good overall, though I haven't had time to review it quite properly yet. I really like this idea, thanks for doing it.
I would add a couple useful options on the revert and the patch, that one can use
options='--all'
with revert and rollback (instead of having to know the file name) and importing patch withoptions='--no-commit'
which is VERY useful until one learns how to use queues.