[PATCH v3] pullrequests: add PullRequest.nice_id class method

[Thomas De Schampheleire]

On Sun, May 3, 2015 at 10:24 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 05/01/2015 09:34 PM, Thomas De Schampheleire wrote:
>>
>> On Fri, May 1, 2015 at 9:30 PM, Thomas De Schampheleire
>> <patrickdepinguin at gmail.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>> # Date 1429265659 -7200
>>> #      Fri Apr 17 12:14:19 2015 +0200
>>> # Node ID f4093cdbf80ae12e9932d69f4ec45ada2afac543
>>> # Parent  b5e399286ce50f2daf791b154653a536431a0205
>>> pullrequests: add PullRequest.nice_id class method
>>>
>
>>> @@ -735,13 +735,16 @@
>>>           return group_name
>>>
>>>       def get_pull_request():
>>> -        pull_request_id = action_params
>>> +        pull_request_id = int(action_params)
>
>
> I'm not sure about this new cast - dropped for now.

This was to fix an error when action_params is a unicode string. It is
really caused by the nice_id method in my patch using %d instead of
%s, which in turn was more by 'coincidence' rather than specific
consideration. So no problem to drop the cast, since you changed the
%s already.

>
>>> --- a/kallithea/model/db.py
>>> +++ b/kallithea/model/db.py
>>> @@ -1387,9 +1387,10 @@
>>>               pr_id = pr_repo = None
>>>               if stat.pull_request:
>>>                   pr_id = stat.pull_request.pull_request_id
>>> +                pr_nice_id = PullRequest.nice_id(pr_id)
>>>                   pr_repo = stat.pull_request.other_repo.repo_name
>>>               grouped[stat.revision] = [str(stat.status),
>>> stat.status_lbl,
>>> -                                      pr_id, pr_repo]
>>> +                                      pr_id, pr_repo, pr_nice_id]
>
>
> I fixed the missing initialization of pr_nice_id.

thanks.

>
>>> +    @classmethod
>>> +    def nice_id(cls, pull_request_id):
>>> +        '''Return a string reference to this pull request'''
>>> +        return '#%d' % pull_request_id
>>> +
>>>       def __json__(self):
>>>           return dict(
>>>               revisions=self.revisions
>
>
>>>   <%def name="breadcrumbs_links()">
>>> -    ${_('Pull request #%s from %s#%s') %
>>> (c.pull_request.pull_request_id, c.pull_request.org_repo.repo_name,
>>> c.cs_branch_name)}
>>> +    ${_('Pull request %s from %s#%s') %
>>> (c.pull_request.nice_id(c.pull_request.pull_request_id),
>>> c.pull_request.org_repo.repo_name, c.cs_branch_name)}
>>
>>
>> This is one particular change I'm not fully sure of: here we're
>> calling the class method 'nice_id' through an actual object
>> c.pull_request. Is this OK? It works, because the cls attribute in
>> nice_id isn't actually used.
>
>
> Hmm ... yeah. I did some search and replace and pushed a compromise between
> your approaches. Thanks for exploring the options.
>
> I will let nice_id be a method that can be used from templates and elsewhere
> and have make_nice_id as class method / function doing the actual
> formatting.
>
> Pushed with these modifications.

Great, thanks!
Thomas