[PATCH 3 of 4] pullrequests: add pull_request_nice_id
Thomas De Schampheleire
patrickdepinguin at gmail.com
Mon Apr 27 04:16:52 EDT 2015
On Mon, Apr 27, 2015 at 1:01 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 04/25/2015 08:33 PM, Thomas De Schampheleire 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 3d17f0d2a96eac41e6bf4312df1f6af476cce4ad
>> # Parent dc58eab8ff5070ab035f53fc241c924dd9aae3fb
>> pullrequests: add pull_request_nice_id
>>
>> Currently, a pull request id is referenced as #5, and the '#' symbol is
>> fixed and repeated in several places. This commit adds a new property
>> pull_request_nice_id, that returns a string reference to the pull request,
>> currently in the form '#5'.
>>
>> This property could be overridden by an organization if they need
>> references
>> in another form, for example PR-5.
>>
>> diff --git a/kallithea/lib/helpers.py b/kallithea/lib/helpers.py
>> --- a/kallithea/lib/helpers.py
>> +++ b/kallithea/lib/helpers.py
>> @@ -423,7 +423,7 @@
>>
>> #==============================================================================
>> from kallithea.lib.vcs.utils import author_name, author_email
>> from kallithea.lib.utils2 import credentials_filter, age as _age
>> -from kallithea.model.db import User, ChangesetStatus
>> +from kallithea.model.db import User, ChangesetStatus, PullRequest
>> age = lambda x, y=False: _age(x, y)
>> capitalize = lambda x: x.capitalize()
>> @@ -736,12 +736,18 @@
>> def get_pull_request():
>> pull_request_id = action_params
>> + pr = PullRequest.get(pull_request_id)
>> + # Fallback when pull request does not exist. This should normally
>> only
>> + # occur in tests with prepopulated fixtures.
>
>
> (It is kind of obvious _what_ is going on; that a default is needed when the
> PR can't be found. The part about _why_ we need to handle it is however very
> relevant. I would guess it would be better to use get_or_404 and fix the
> tests.)
The test is not so easy to fix I'm afraid: the fixture in question is
kallithea/tests/fixtures/journal_dump.csv, which is a dump of the
admin journal, containing a number of references to pull requests that
do not actually exist in the database.
To fix that means not using any dump here, meaning we need to
dynamically create all the entries in the database. Not only would
this mean slower test execution, but also mixing testing the admin
journal itself from creating entries for various elements in the
database itself.
>>
>> + nice_id = pr.pull_request_nice_id if pr else '#%s' %
>> pull_request_id
>
>
> Hmm ... instead of hardcoding the default format here, how about making
> pull_request_nice_id a class method (function of pr id) so it can be used
> for non-existing PRs? Perhaps better: just make it 'UNKNOWN#%s' ...
I like the idea of a class method, I hadn't thought about that. This
has the big advantage that you do not need any database access to get
the nice id, while the get() did. You cannot determine invalid IDs,
but as mentioned in the comment the ID is normally coming from a sane
place; if not then clicking the corresponding link will open a page
that will do a get_or_404 anyway.
>
>
>> +
>> deleted = user_log.repository is None
>> if deleted:
>> repo_name = user_log.repository_name
>> else:
>> repo_name = user_log.repository.repo_name
>> - return link_to(_('Pull request #%s') % pull_request_id,
>> +
>> + return link_to(_('Pull request %s') % nice_id,
>> url('pullrequest_show', repo_name=repo_name,
>> pull_request_id=pull_request_id))
>> @@ -1414,7 +1420,6 @@
>> def changeset_status_lbl(changeset_status):
>> return dict(ChangesetStatus.STATUSES).get(changeset_status)
>> -
>> def get_permission_name(key):
>> return dict(Permission.PERMS).get(key)
>> diff --git a/kallithea/model/comment.py b/kallithea/model/comment.py
>> --- a/kallithea/model/comment.py
>> +++ b/kallithea/model/comment.py
>> @@ -130,9 +130,9 @@
>> comment_url = pull_request.url(canonical=True,
>> anchor='comment-%s' % comment.comment_id)
>> subj = safe_unicode(
>> - h.link_to('Re pull request #%(pr_id)s: %(desc)s %(line)s'
>> % \
>> + h.link_to('Re pull request %(pr_id)s: %(desc)s %(line)s'
>> % \
>> {'desc': desc,
>> - 'pr_id': comment.pull_request.pull_request_id,
>> + 'pr_id':
>> comment.pull_request.pull_request_nice_id,
>
>
> But ... pr_id is no longer a pr_id but a nice_id? It is nice to keep the
> diff small but we should try to stay consistent. Please use a name like
> pr_nice_id in all the places where the type no longer is a pr id.
OK
>
>
>> 'line': line},
>> comment_url)
>> )
>> @@ -148,7 +148,7 @@
>> #set some variables for email notification
>> email_kwargs = {
>> 'pr_title': pull_request.title,
>> - 'pr_id': pull_request.pull_request_id,
>> + 'pr_id': pull_request.pull_request_nice_id,
>> 'status_change': status_change,
>> 'closing_pr': closing_pr,
>> 'pr_comment_url': comment_url,
>> diff --git a/kallithea/model/db.py b/kallithea/model/db.py
>> --- 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 = stat.pull_request.pull_request_nice_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]
>> return grouped
>> def _repo_size(self):
>> @@ -2305,6 +2306,11 @@
>> .first()
>> return str(status.status) if status else ''
>> + @property
>> + def pull_request_nice_id(self):
>> + '''Return a string reference to this pull request'''
>> + return '#%d' % self.pull_request_id
>
>
> We talked about it and I might have suggested the name ... but it is a very
> long.
>
> The database model generally gives table X the primary key field X_id. It is
> a long name with redundancy but convenient when defining and debugging
> queries.
>
> I do however think it would be fine to just call this property nice_id.
OK :)
Thanks for the feedback,
Thomas
More information about the kallithea-general
mailing list