Patchwork push: hg should provide hint when no paths configured (issue3692)

# HG changeset patch
# User anuraggoel <anurag.dsps@gmail.com>
# Date 1392116931 -19800
#      Tue Feb 11 16:38:51 2014 +0530
# Node ID 6bda034329f21f522b4bb2498bae4c84f3653569
# Parent  fff0a71f8177be4ca634784d8b603c8e4d6c3601
push: hg should provide hint when no paths configured (issue3692)

Hint will be provided to the user if default path is not configured when user type "hg push" command.
I also run the code on the test cases this time, all seems to work fine.

On 02/11/2014 12:12 PM, Anurag Goel wrote:
> # HG changeset patch
> # User anuraggoel <anurag.dsps@gmail.com>
> # Date 1392116931 -19800
> #      Tue Feb 11 16:38:51 2014 +0530
> # Node ID 6bda034329f21f522b4bb2498bae4c84f3653569
> # Parent  fff0a71f8177be4ca634784d8b603c8e4d6c3601
> push: hg should provide hint when no paths configured (issue3692)
>
> Hint will be provided to the user if default path is not configured when user type "hg push" command.
> I also run the code on the test cases this time, all seems to work fine.

This patch is getting better!

> diff -r fff0a71f8177 -r 6bda034329f2 mercurial/commands.py
> --- a/mercurial/commands.py	Thu Feb 06 02:17:48 2014 +0100
> +++ b/mercurial/commands.py	Tue Feb 11 16:38:51 2014 +0530
> @@ -4711,7 +4711,16 @@
>       dest, branches = hg.parseurl(dest, opts.get('branch'))
>       ui.status(_('pushing to %s\n') % util.hidepassword(dest))
>       revs, checkout = hg.addbranchrevs(repo, repo, branches, opts.get('rev'))
> -    other = hg.peer(repo, opts, dest)
> +    if dest == "default-push":
> +        try:
> +            other = hg.peer(repo, opts, dest)
> +        except error.RepoError:
> +            error1 = (_("repository default-push not found!"))

Why the extra ()? The 1 suffix also seems odd. I would call it "msg" 
like it is one in at least one or place in that file ... or do like most 
other places and just let the Abort command span multiple lines.

> +            hint = _("see the \"path\" section in \"hg help config\"")
> +            raise util.Abort(error1, hint=hint)
> +    else:
> +        other = hg.peer(repo,opts,dest)

This line proves that you haven't run the test suite. Step 9 on 
http://mercurial.selenic.com/wiki/ContributingChanges .

Also, how about adding some test coverage of this? You already got the 
link to http://mercurial.selenic.com/wiki/WritingTests .

One important test case: having a default-path configuration that points 
at a non-existing location.

/Mads

On Wed, 2014-02-12 at 01:24 +0530, Anurag Goel wrote:
> For the above patch, i have fixed the changes which you told me to do so
> and also run the test suits but get some weird result.
> 
> When I type "make tests" ,it stuck into some loop but when i test each file
> individually, it works fine.
> 
> My query is how did you get that i hadn't run the test suite and what do
> mean by phrase "just let the Abort command span multiple lines" ?

The test suite runs check-code, which checks your coding style. That
line contains a coding style error.

You could have written this as:

raise util.Abort(_("some long string...."),
                 hint=_("some other long string"))

For the above patch, i have fixed the changes which you told me to do so
and also run the test suits but get some weird result.

When I type "make tests" ,it stuck into some loop but when i test each file
individually, it works fine.

My query is how did you get that i hadn't run the test suite and what do
mean by phrase "just let the Abort command span multiple lines" ?




On Tue, Feb 11, 2014 at 6:10 PM, Mads Kiilerich <mads@kiilerich.com> wrote:

> On 02/11/2014 12:12 PM, Anurag Goel wrote:
>
>> # HG changeset patch
>> # User anuraggoel <anurag.dsps@gmail.com>
>> # Date 1392116931 -19800
>> #      Tue Feb 11 16:38:51 2014 +0530
>> # Node ID 6bda034329f21f522b4bb2498bae4c84f3653569
>> # Parent  fff0a71f8177be4ca634784d8b603c8e4d6c3601
>> push: hg should provide hint when no paths configured (issue3692)
>>
>> Hint will be provided to the user if default path is not configured when
>> user type "hg push" command.
>> I also run the code on the test cases this time, all seems to work fine.
>>
>
> This patch is getting better!
>
>
>  diff -r fff0a71f8177 -r 6bda034329f2 mercurial/commands.py
>> --- a/mercurial/commands.py     Thu Feb 06 02:17:48 2014 +0100
>> +++ b/mercurial/commands.py     Tue Feb 11 16:38:51 2014 +0530
>> @@ -4711,7 +4711,16 @@
>>       dest, branches = hg.parseurl(dest, opts.get('branch'))
>>       ui.status(_('pushing to %s\n') % util.hidepassword(dest))
>>       revs, checkout = hg.addbranchrevs(repo, repo, branches,
>> opts.get('rev'))
>> -    other = hg.peer(repo, opts, dest)
>> +    if dest == "default-push":
>> +        try:
>> +            other = hg.peer(repo, opts, dest)
>> +        except error.RepoError:
>> +            error1 = (_("repository default-push not found!"))
>>
>
> Why the extra ()? The 1 suffix also seems odd. I would call it "msg" like
> it is one in at least one or place in that file ... or do like most other
> places and just let the Abort command span multiple lines.
>
>
>  +            hint = _("see the \"path\" section in \"hg help config\"")
>> +            raise util.Abort(error1, hint=hint)
>> +    else:
>> +        other = hg.peer(repo,opts,dest)
>>
>
> This line proves that you haven't run the test suite. Step 9 on
> http://mercurial.selenic.com/wiki/ContributingChanges .
>
> Also, how about adding some test coverage of this? You already got the
> link to http://mercurial.selenic.com/wiki/WritingTests .
>
> One important test case: having a default-path configuration that points
> at a non-existing location.
>
> /Mads
>