Patchwork [1,of,4,V3] hgweb: add dynamic search function selection, depending on the query

login
register
mail settings
Submitter Alexander Plavin
Date Aug. 22, 2013, 3:09 p.m.
Message ID <779319758a3c5ade2123.1377184167@debian-alexander.dolgopa>
Download mbox | patch
Permalink /patch/2240/
State Superseded
Commit cf9e5e45c1d3ad3f665182cb7576a7a515842f38
Headers show

Comments

Alexander Plavin - Aug. 22, 2013, 3:09 p.m.
# HG changeset patch
# User Alexander Plavin <alexander@plav.in>
# Date 1377175523 -14400
#      Thu Aug 22 16:45:23 2013 +0400
# Node ID 779319758a3c5ade2123dc4c42d76824111b801b
# Parent  a133dfe29d0325c1cea54aebd97599e02b30a861
hgweb: add dynamic search function selection, depending on the query

This allows adding other specific search functions, in addition to current
keyword search.
Augie Fackler - Aug. 26, 2013, 2:43 p.m.
On Thu, Aug 22, 2013 at 07:09:27PM +0400, Alexander Plavin wrote:
> # HG changeset patch
> # User Alexander Plavin <alexander@plav.in>
> # Date 1377175523 -14400
> #      Thu Aug 22 16:45:23 2013 +0400
> # Node ID 779319758a3c5ade2123dc4c42d76824111b801b
> # Parent  a133dfe29d0325c1cea54aebd97599e02b30a861
> hgweb: add dynamic search function selection, depending on the query
>
> This allows adding other specific search functions, in addition to current
> keyword search.
>
> diff -r a133dfe29d03 -r 779319758a3c mercurial/hgweb/webcommands.py
> --- a/mercurial/hgweb/webcommands.py	Wed Jul 24 03:20:26 2013 +0400
> +++ b/mercurial/hgweb/webcommands.py	Thu Aug 22 16:45:23 2013 +0400
> @@ -138,10 +138,17 @@
>
>              yield ctx
>
> +    searchfuncs = {
> +        'kw': keywordsearch,

This magic string gets repeated often, over several changesets. Could
I persuade you to give it a descriptive symbolic name so it's more
obvious what it is to future readers?

> +    }
> +
> +    def getsearchmode():
> +        return 'kw'
> +
>      def changelist(**map):
>          count = 0
>
> -        for ctx in keywordsearch():
> +        for ctx in searchfunc():
>              count += 1
>              n = ctx.node()
>              showtags = webutil.showtag(web.repo, tmpl, 'changelogtag', n)
> @@ -181,6 +188,9 @@
>      morevars['revcount'] = revcount * 2
>      morevars['rev'] = query
>
> +    mode = getsearchmode()
> +    searchfunc = searchfuncs[mode]
> +
>      tip = web.repo['tip']
>      parity = paritygen(web.stripecount)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Alexander Plavin - Aug. 27, 2013, 7:27 a.m.
26.08.2013, 18:44, "Augie Fackler" <raf@durin42.com>:
> On Thu, Aug 22, 2013 at 07:09:27PM +0400, Alexander Plavin wrote:
>
>>  # HG changeset patch
>>  # User Alexander Plavin <alexander@plav.in>
>>  # Date 1377175523 -14400
>>  #      Thu Aug 22 16:45:23 2013 +0400
>>  # Node ID 779319758a3c5ade2123dc4c42d76824111b801b
>>  # Parent  a133dfe29d0325c1cea54aebd97599e02b30a861
>>  hgweb: add dynamic search function selection, depending on the query
>>
>>  This allows adding other specific search functions, in addition to current
>>  keyword search.
>>
>>  diff -r a133dfe29d03 -r 779319758a3c mercurial/hgweb/webcommands.py
>>  --- a/mercurial/hgweb/webcommands.py Wed Jul 24 03:20:26 2013 +0400
>>  +++ b/mercurial/hgweb/webcommands.py Thu Aug 22 16:45:23 2013 +0400
>>  @@ -138,10 +138,17 @@
>>
>>               yield ctx
>>
>>  +    searchfuncs = {
>>  +        'kw': keywordsearch,
>
> This magic string gets repeated often, over several changesets. Could
> I persuade you to give it a descriptive symbolic name so it's more
> obvious what it is to future readers?

Done. (assuming you mean 'kw', replaced with 'keyword')

>
>>  +    }
>>  +
>>  +    def getsearchmode():
>>  +        return 'kw'
>>  +
>>       def changelist(**map):
>>           count = 0
>>
>>  -        for ctx in keywordsearch():
>>  +        for ctx in searchfunc():
>>               count += 1
>>               n = ctx.node()
>>               showtags = webutil.showtag(web.repo, tmpl, 'changelogtag', n)
>>  @@ -181,6 +188,9 @@
>>       morevars['revcount'] = revcount * 2
>>       morevars['rev'] = query
>>
>>  +    mode = getsearchmode()
>>  +    searchfunc = searchfuncs[mode]
>>  +
>>       tip = web.repo['tip']
>>       parity = paritygen(web.stripecount)
>>
>>  _______________________________________________
>>  Mercurial-devel mailing list
>>  Mercurial-devel@selenic.com
>>  http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Aug. 27, 2013, 8:15 p.m.
On Tue, Aug 27, 2013 at 3:27 AM, Alexander Plavin <alexander@plav.in> wrote:
>> This magic string gets repeated often, over several changesets. Could
>> I persuade you to give it a descriptive symbolic name so it's more
>> obvious what it is to future readers?
>
> Done. (assuming you mean 'kw', replaced with 'keyword')


I meant something like

_KEYWORD_SEARCH = 'kw'

and then using the symbolic constant throughout.
Alexander Plavin - Aug. 28, 2013, 9:37 a.m.
28.08.2013, 00:15, "Augie Fackler" <raf@durin42.com>:
> On Tue, Aug 27, 2013 at 3:27 AM, Alexander Plavin <alexander@plav.in> wrote:
>
>>>  This magic string gets repeated often, over several changesets. Could
>>>  I persuade you to give it a descriptive symbolic name so it's more
>>>  obvious what it is to future readers?
>>  Done. (assuming you mean 'kw', replaced with 'keyword')
>
> I meant something like
>
> _KEYWORD_SEARCH = 'kw'
>
> and then using the symbolic constant throughout.

I haven't noticed a place where such thing is used in Mercurial, could you please point at one of them (if there are any)?
Augie Fackler - Aug. 28, 2013, 12:24 p.m.
On Wed, Aug 28, 2013 at 5:37 AM, Alexander Plavin <alexander@plav.in> wrote:
> I haven't noticed a place where such thing is used in Mercurial, could you please point at one of them (if there are any)?

I guess there aren't. Hm. Kevin, do you have an opinion?
Laurens Holst - Aug. 28, 2013, 1:20 p.m.
Op 27-08-13 22:15, Augie Fackler schreef:
> On Tue, Aug 27, 2013 at 3:27 AM, Alexander Plavin <alexander@plav.in> wrote:
>>> This magic string gets repeated often, over several changesets. Could
>>> I persuade you to give it a descriptive symbolic name so it's more
>>> obvious what it is to future readers?
>> Done. (assuming you mean 'kw', replaced with 'keyword')
>
> I meant something like
>
> _KEYWORD_SEARCH = 'kw'
>
> and then using the symbolic constant throughout.

Doesn’t a readable non abbreviated string serve pretty much the same 
purpose?

~Laurens
Martin Geisler - Sept. 1, 2013, 8:18 p.m.
Alexander Plavin <alexander@plav.in> writes:

> 01.09.2013, 08:45, "Kevin Bullock" <kbullock+mercurial@ringworld.org>:
>> On 28 Aug 2013, at 7:24 AM, Augie Fackler wrote:
>>
>>>  On Wed, Aug 28, 2013 at 5:37 AM, Alexander Plavin <alexander@plav.in> wrote:
>>>>  I haven't noticed a place where such thing is used in Mercurial, could you please point at one of them (if there are any)?
>>>  I guess there aren't. Hm. Kevin, do you have an opinion?
>>
>> I always have an opinion; sometimes it's even warranted :P
>>
>> A symbolic name would guard against typos in the string literal. I'm
>> in favor.
>
> Would it, really? Due to Python being interpreted, an exception would
> be raised only when the line of code executes, with both methods (a
> separate constant, or a string literal).

That is true, but Pyflakes can detect undefined variables most of the
time.
Alexander Plavin - Sept. 2, 2013, 5:36 p.m.
02.09.2013, 00:18, "Martin Geisler" <martin@geisler.net>:
> Alexander Plavin <alexander@plav.in> writes:
>
>>  01.09.2013, 08:45, "Kevin Bullock" <kbullock+mercurial@ringworld.org>:
>>>  On 28 Aug 2013, at 7:24 AM, Augie Fackler wrote:
>>>>   On Wed, Aug 28, 2013 at 5:37 AM, Alexander Plavin <alexander@plav.in> wrote:
>>>>>   I haven't noticed a place where such thing is used in Mercurial, could you please point at one of them (if there are any)?
>>>>   I guess there aren't. Hm. Kevin, do you have an opinion?
>>>  I always have an opinion; sometimes it's even warranted :P
>>>
>>>  A symbolic name would guard against typos in the string literal. I'm
>>>  in favor.
>>  Would it, really? Due to Python being interpreted, an exception would
>>  be raised only when the line of code executes, with both methods (a
>>  separate constant, or a string literal).
>
> That is true, but Pyflakes can detect undefined variables most of the
> time.

Hm, may be this really makes some sense... How would it be better to do this? Just have the constants defined at the top, and use them in the code? I ask because there is no place where such technique is used in hg now.

>
> --
> Martin Geisler
Augie Fackler - Sept. 3, 2013, 3:51 a.m.
On Sep 2, 2013, at 1:36 PM, Alexander Plavin <alexander@plav.in> wrote:

> Just have the constants defined at the top, and use them in the code?

That's what I do with my code at work, and it's been pretty helpful.

AF
Alexander Plavin - Sept. 3, 2013, 8:23 p.m.
And what about the patch already being crewed? Should I send a new version or a patch to this patch?

03.09.2013, 07:52, "Augie Fackler" <raf@durin42.com>:
> On Sep 2, 2013, at 1:36 PM, Alexander Plavin <alexander@plav.in> wrote:
>
>>  Just have the constants defined at the top, and use them in the code?
>
> That's what I do with my code at work, and it's been pretty helpful.
>
> AF
Augie Fackler - Sept. 3, 2013, 8:23 p.m.
On Tue, Sep 3, 2013 at 4:23 PM, Alexander Plavin <alexander@plav.in> wrote:
> And what about the patch already being crewed? Should I send a new version or a patch to this patch?


Patch to this patch is fine.

Patch

diff -r a133dfe29d03 -r 779319758a3c mercurial/hgweb/webcommands.py
--- a/mercurial/hgweb/webcommands.py	Wed Jul 24 03:20:26 2013 +0400
+++ b/mercurial/hgweb/webcommands.py	Thu Aug 22 16:45:23 2013 +0400
@@ -138,10 +138,17 @@ 
 
             yield ctx
 
+    searchfuncs = {
+        'kw': keywordsearch,
+    }
+
+    def getsearchmode():
+        return 'kw'
+
     def changelist(**map):
         count = 0
 
-        for ctx in keywordsearch():
+        for ctx in searchfunc():
             count += 1
             n = ctx.node()
             showtags = webutil.showtag(web.repo, tmpl, 'changelogtag', n)
@@ -181,6 +188,9 @@ 
     morevars['revcount'] = revcount * 2
     morevars['rev'] = query
 
+    mode = getsearchmode()
+    searchfunc = searchfuncs[mode]
+
     tip = web.repo['tip']
     parity = paritygen(web.stripecount)