This is an archive of the discontinued Mercurial Phabricator instance.

Differential D812

tweakdefaults: explicitly enable/disable single colon warning
ClosedPublic

Authored by ryanmce on Sep 25 2017, 5:31 AM.
Tags
None
Subscribers
singhsrb, quark

Details

Reviewers
mbthomas
durham
singhsrb
Group Reviewers
Restricted Project
Commits
rFBHGXfb29ea176fba: tweakdefaults: explicitly enable/disable single colon warning
Summary

I think this is a better way of surfacing this warning. It also allows
us to tweak the message so we can point at new documentation I have already
written up.

Test Plan

Updated test

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
arcpatch-D812 (bookmark) on default (branch)
Lint
Lint OK
Unit
Unit Test Errors

Event Timeline

ryanmce created this revision.Sep 25 2017, 5:31 AM
mbthomas accepted this revision.Sep 25 2017, 2:41 PM
This revision is now accepted and ready to land.Sep 25 2017, 2:41 PM
mbthomas added inline comments.Sep 25 2017, 2:43 PM
hgext3rd/tweakdefaults.py
705–708

You should add a test for setting this option.

durham accepted this revision.Sep 26 2017, 7:46 AM
singhsrb added subscribers: quark, singhsrb.Sep 26 2017, 1:44 PM
singhsrb requested changes to this revision.

In my opinion, _analyzewrapper can be extended to do additional analysis over the analysis it does right now. So, I prefer having develwarn config option as in D802 which disables all the analysis. It also seems fine to have both the config options: develwarn and singlecolonwarn.

tests/test-tweakdefaults.t
1

This is bad use for test cases since only a portion of the test is executed conditionally and the rest of the test is executed twice leading to a lot of redundancy. I added the test cases in D802 without realizing this and therefore, fixed this in D814. I think for the testing (as confirmed by @quark):

  • We can either split this into a separate test for developer warnings and then use the testcases approach.
  • Or, use the '--config' flag as used in D814.
This revision now requires changes to proceed.Sep 26 2017, 1:44 PM
quark added a comment.Sep 26 2017, 2:31 PM

I think the : warning is mainly for users, not (Mercurial) developers. So I prefer the explicit singlecolonwarn name. It also feels more consistent with other tweakdefaults config options.

There is a devel.all-warnings config in core Mercurial which is for developer warnings.

It seems someone has to handle some merge conflicts. It seems easier practically to apply D814 first, then do a replace and run run-tests.py -i.

quark updated this revision to Diff 2093.Sep 26 2017, 5:01 PM
quark added a comment.Sep 26 2017, 5:03 PM

@ryanmce, @singhsrb I have pushed D814 and updated this diff. Let me know if this looks better.

singhsrb accepted this revision.Sep 26 2017, 5:07 PM

Ahh, I wasn't aware of devel.all-warnings. Thanks for taking care of the merge!

This revision is now accepted and ready to land.Sep 26 2017, 5:07 PM
ryanmce added a comment.Sep 27 2017, 8:41 AM

This is what I was about to do. Thanks for taking care of it for me @quark!

ryanmce updated this revision to Diff 2116.Sep 27 2017, 8:50 AM

add test

Closed by commit rFBHGXfb29ea176fba: tweakdefaults: explicitly enable/disable single colon warning (authored by ryanmce). · Explain WhySep 27 2017, 9:28 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/tweakdefaults.py (36 lines)
Mtests/test-tweakdefaults.t (39 lines)

Diff 2116

hgext3rd/tweakdefaults.py

allowfullrepohistgrep = False allowfullrepohistgrep = False
allowmerge = True allowmerge = True
allowrollback = True allowrollback = True
allowtags = True allowtags = True
# change rebase exit from 1 to 0 if nothing is rebased # change rebase exit from 1 to 0 if nothing is rebased
nooprebase = True nooprebase = True
# whether to show a warning on some deprecated usages
singlecolonwarn = False
# educational messages # educational messages
bmnodesthint = '' bmnodesthint = ''
bmnodestmsg = '' bmnodestmsg = ''
branchmessage = '' branchmessage = ''
branchesmessage = '' branchesmessage = ''
mergemessage = '' mergemessage = ''
nodesthint = '' nodesthint = ''
nodestmsg = '' nodestmsg = ''
rollbackhint = '' rollbackhint = ''
singlecolonmsg = ''
tagsmessage = '' tagsmessage = ''
# output new hashes when nodes get updated # output new hashes when nodes get updated
showupdated = False showupdated = False
# Show developers warning for using discouraged features.
#
# For example, 'x:y' in revsets shows revision numbers between x and y,
# both inclusive. Usually, this doesn't correspond to any meaningful
# interpretation of the repo and is therefore, discouraged.
develwarn = True
""" """
from __future__ import absolute_import from __future__ import absolute_import
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial.node import ( from mercurial.node import (
short, short,
) )
from mercurial import ( from mercurial import (
opawarerebase = markermetadatawritingcommand(ui, _rebase, 'rebase') opawarerebase = markermetadatawritingcommand(ui, _rebase, 'rebase')
wrapcommand(rebase.cmdtable, 'rebase', opawarerebase) wrapcommand(rebase.cmdtable, 'rebase', opawarerebase)
wrapfunction(scmutil, 'cleanupnodes', cleanupnodeswrapper) wrapfunction(scmutil, 'cleanupnodes', cleanupnodeswrapper)
entry = wrapcommand(commands.table, 'pull', pull) entry = wrapcommand(commands.table, 'pull', pull)
options = entry[1] options = entry[1]
options.append( options.append(
('d', 'dest', '', _('destination for rebase or update'))) ('d', 'dest', '', _('destination for rebase or update')))
if ui.configbool('tweakdefaults', 'develwarn', True):
# anonymous function to pass ui object to _analyzewrapper # anonymous function to pass ui object to _analyzewrapper
def _analyzewrap(orig, x): def _analyzewrap(orig, x):
return _analyzewrapper(orig, x, ui) return _analyzewrapper(orig, x, ui)
wrapfunction(revsetlang, '_analyze', _analyzewrap) wrapfunction(revsetlang, '_analyze', _analyzewrap)
try: try:
rebaseext = extensions.find('rebase') rebaseext = extensions.find('rebase')
# tweakdefaults is already loaded before other extensions # tweakdefaults is already loaded before other extensions
# (see tweakorder() function) so if these functions are wrapped # (see tweakorder() function) so if these functions are wrapped
# by something else, it's not a problem. # by something else, it's not a problem.
wrapfunction(rebaseext, '_computeobsoletenotrebased', wrapfunction(rebaseext, '_computeobsoletenotrebased',
_computeobsoletenotrebasedwrapper) _computeobsoletenotrebasedwrapper)
wrapfunction(rebaseext, '_checkobsrebase', wrapfunction(rebaseext, '_checkobsrebase',
def cmd(*args, **kwargs): def cmd(*args, **kwargs):
repo = args[repo_index] repo = args[repo_index]
overrides = {(globaldata, createmarkersoperation): operationame} overrides = {(globaldata, createmarkersoperation): operationame}
with repo.ui.configoverride(overrides, 'tweakdefaults'): with repo.ui.configoverride(overrides, 'tweakdefaults'):
return origcmd(*args, **kwargs) return origcmd(*args, **kwargs)
return cmd return cmd
def _analyzewrapper(orig, x, ui): def _analyzewrapper(orig, x, ui):
"""Wraps analyzer to detect the use of colons in the revisions """Wraps analyzer to detect the use of colons in the revisions"""
"""
result = orig(x) result = orig(x)
if isinstance(x, tuple) and \ enabled = ui.configbool('tweakdefaults', 'singlecolonwarn')
(x[0] in ('range', 'rangepre', 'rangepost')) and \
x != ('rangepre', ('symbol', '.')):
# The last condition is added so that warnings are not shown if # The last condition is added so that warnings are not shown if
# hg log --follow is invoked w/o arguments # hg log --follow is invoked w/o arguments
ui.develwarn("use of ':' is deprecated\n") if enabled and isinstance(x, tuple) and \
(x[0] in ('range', 'rangepre', 'rangepost')) and \
x != ('rangepre', ('symbol', '.')):
msg = ui.config('tweakdefaults', 'singlecolonmsg',
_("warning: use of ':' is deprecated"))
ui.warn('%s\n' % msg)
mbthomasUnsubmitted
Not Done

You should add a test for setting this option.

mbthomas: You should add a test for setting this option.
return result return result
def _rebase(orig, ui, repo, **opts): def _rebase(orig, ui, repo, **opts):
if not opts.get('date') and not ui.configbool('tweakdefaults', if not opts.get('date') and not ui.configbool('tweakdefaults',
'rebasekeepdate'): 'rebasekeepdate'):
opts['date'] = currentdate() opts['date'] = currentdate()
if opts.get('continue') or opts.get('abort') or opts.get('restack'): if opts.get('continue') or opts.get('abort') or opts.get('restack'):

tests/test-tweakdefaults.t

$ . "$TESTDIR/histedit-helpers.sh" $ . "$TESTDIR/histedit-helpers.sh"
singhsrbUnsubmitted
Not Done

This is bad use for test cases since only a portion of the test is executed conditionally and the rest of the test is executed twice leading to a lot of redundancy. I added the test cases in D802 without realizing this and therefore, fixed this in D814. I think for the testing (as confirmed by @quark):

  • We can either split this into a separate test for developer warnings and then use the testcases approach.
  • Or, use the '--config' flag as used in D814.
singhsrb: This is bad use for test cases since only a portion of the test is executed conditionally and…
$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [extensions] > [extensions]
> tweakdefaults=$TESTDIR/../hgext3rd/tweakdefaults.py > tweakdefaults=$TESTDIR/../hgext3rd/tweakdefaults.py
> fbamend=$TESTDIR/../hgext3rd/fbamend > fbamend=$TESTDIR/../hgext3rd/fbamend
> rebase= > rebase=
> histedit= > histedit=
> [experimental] > [experimental]
x 3 x 3
| |
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
Test that developer warning is shown whenever ':' is used implicitly or explicitly Test that warning is shown whenever ':' is used with singlecolonwarn set
$ hg log -G -T '{rev} {bookmarks}' -r '0:2' $ hg log -G -T '{rev} {bookmarks}' -r '0:2' --config tweakdefaults.singlecolonwarn=1
devel-warn: use of ':' is deprecated warning: use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
$ hg log -G -T '{rev} {bookmarks}' -r '0:2'
$ hg log -G -T '{rev} {bookmarks}' -r '0:2' --config tweakdefaults.develwarn=False
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
$ hg log -G -T '{rev} {bookmarks}' -r ':2' --config tweakdefaults.singlecolonwarn=1
warning: use of ':' is deprecated
o 2
|
o 1 master
|
o 0
$ hg log -G -T '{rev} {bookmarks}' -r ':2' $ hg log -G -T '{rev} {bookmarks}' -r ':2'
devel-warn: use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
$ hg log -G -T '{rev} {bookmarks}' -r '0:' --config tweakdefaults.singlecolonwarn=1
$ hg log -G -T '{rev} {bookmarks}' -r ':2' --config tweakdefaults.develwarn=False warning: use of ':' is deprecated
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
$ hg log -G -T '{rev} {bookmarks}' -r '0:' $ hg log -G -T '{rev} {bookmarks}' -r '0:'
devel-warn: use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
$ hg log -G -T '{rev} {bookmarks}' -r '0:' --config tweakdefaults.develwarn=False In this testcase warning should not be shown
$ hg log -G -T '{rev} {bookmarks}' -r ':' --config tweakdefaults.singlecolonwarn=1
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
Check that the custom message can be used
In this testcase warning should not be shown $ hg log -G -T '{rev} {bookmarks}' -r '0:' --config tweakdefaults.singlecolonwarn=1 --config tweakdefaults.singlecolonmsg="hey stop that"
$ hg log -G -T '{rev} {bookmarks}' -r ':' hey stop that
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0