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
default
Lint
Lint OK
Unit
Unit Tests OK

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
712–713

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 (19 lines)
Mtests/test-tweakdefaults.t (21 lines)

Diff 2062

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. # Show developers warning for using discouraged features.
# #
# For example, 'x:y' in revsets shows revision numbers between x and y, # For example, 'x:y' in revsets shows revision numbers between x and y,
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',
_("use of ':' is deprecated"))
mbthomasUnsubmitted
Not Done

You should add a test for setting this option.

mbthomas: You should add a test for setting this option.
ui.warn('%s\n' % msg)
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()

tests/test-tweakdefaults.t

#testcases develwarn.true develwarn.false #testcases singlecolonwarn.true singlecolonwarn.false
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…
$ . "$TESTDIR/histedit-helpers.sh" $ . "$TESTDIR/histedit-helpers.sh"
$ 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]
> updatecheck=noconflict > updatecheck=noconflict
> EOF > EOF
#if develwarn.false #if singlecolonwarn.true
$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [tweakdefaults] > [tweakdefaults]
> develwarn = False > singlecolonwarn = True
> EOF > EOF
#endif #endif
Setup repo Setup repo
$ hg init repo $ hg init repo
$ cd repo $ cd repo
$ touch a $ touch a
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 developer warning is shown whenever ':' is used implicitly or explicitly
#if develwarn.true #if singlecolonwarn.true
$ hg log -G -T '{rev} {bookmarks}' -r '0:2' $ hg log -G -T '{rev} {bookmarks}' -r '0:2'
devel-warn: use of ':' is deprecated use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
#else #else
$ hg log -G -T '{rev} {bookmarks}' -r '0:2' $ hg log -G -T '{rev} {bookmarks}' -r '0:2'
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
#endif #endif
#if develwarn.true #if singlecolonwarn.true
$ hg log -G -T '{rev} {bookmarks}' -r ':2' $ hg log -G -T '{rev} {bookmarks}' -r ':2'
devel-warn: use of ':' is deprecated use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
#else #else
$ hg log -G -T '{rev} {bookmarks}' -r ':2' $ hg log -G -T '{rev} {bookmarks}' -r ':2'
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
#endif #endif
#if develwarn.true #if singlecolonwarn.true
$ hg log -G -T '{rev} {bookmarks}' -r '0:' $ hg log -G -T '{rev} {bookmarks}' -r '0:'
devel-warn: use of ':' is deprecated use of ':' is deprecated
at: *tweakdefaults.py:* (_analyzewrap) (glob)
o 2 o 2
| |
o 1 master o 1 master
| |
o 0 o 0
#else #else
$ hg log -G -T '{rev} {bookmarks}' -r '0:' $ hg log -G -T '{rev} {bookmarks}' -r '0:'