[PATCH V3] summary, changelog, compare: make the comments counts more accurate
Angel Ezquerra
angel.ezquerra at gmail.com
Tue Mar 1 22:49:05 UTC 2016
On Mon, Feb 29, 2016 at 4:28 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 02/26/2016 09:54 PM, Angel Ezquerra wrote:
>
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra at gmail.com>
> # Date 1456382571 -3600
> # Thu Feb 25 07:42:51 2016 +0100
> # Branch stable
> # Node ID a13394a91f352a982afcf0b9141ece5537caacb1
> # Parent 17dd4d5bff87a60b34db82e8ae9675acac138fee
> summary, changelog, compare: make the comments counts more accurate
>
> Up until now the comment counts shown on the summary, changelog and compare
> pages counted both actual (non-empty) comments and status changes. This was
> not
> particularly useful, since the status information is already shown on the
> status icon. Also, openeing a changeset page to find out that it just had
> its
> status changed (with no textual comments) is quite annoying.
>
> Instead, show the number of "actual" comments (i.e. those that have some
> comment
> text).
>
> diff --git a/kallithea/controllers/changelog.py
> b/kallithea/controllers/changelog.py
> --- a/kallithea/controllers/changelog.py
> +++ b/kallithea/controllers/changelog.py
> @@ -63,6 +63,7 @@
> page_revisions = [x.raw_id for x in list(c.repo_changesets)]
> c.comments = c.db_repo.get_comments(page_revisions)
> c.statuses = c.db_repo.statuses(page_revisions)
> + c.comment_counts = c.db_repo.count_comments(c.comments)
>
>
> It seems like an odd pattern to retrieve a value from a c.db_repo method ...
> and then pass the same value back to another c.db_repo method for further
> processing. The latter could perhaps be a class function?
>
> (Other high level comment functions live in the comment model like
> get_inline_comments is. It is already a bit inconsistent one way or the
> other that get_comments lives in db. Our MVC model is quite ... not MVC.)
True. I wanted to avoid having to compute the comment list again. I
can change it into a class function or put it somewhere else as you
suggest.
> class ChangelogController(BaseRepoController):
> @@ -153,6 +154,7 @@
> page_revisions = [x.raw_id for x in c.pagination]
> c.comments = c.db_repo.get_comments(page_revisions)
> c.statuses = c.db_repo.statuses(page_revisions)
> + c.comment_counts = c.db_repo.count_comments(c.comments)
> except EmptyRepositoryError as e:
> h.flash(safe_str(e), category='warning')
> return redirect(url('summary_home', repo_name=c.repo_name))
> diff --git a/kallithea/controllers/compare.py
> b/kallithea/controllers/compare.py
> --- a/kallithea/controllers/compare.py
> +++ b/kallithea/controllers/compare.py
> @@ -238,6 +238,7 @@
> raw_ids = [x.raw_id for x in c.cs_ranges]
> c.cs_comments = other_repo.get_comments(raw_ids)
> c.statuses = other_repo.statuses(raw_ids)
> + c.cs_comment_counts = c.db_repo.count_comments(c.cs_comments)
>
> revs = [ctx.revision for ctx in reversed(c.cs_ranges)]
> c.jsdata = json.dumps(graph_data(c.cs_repo.scm_instance, revs))
> diff --git a/kallithea/model/db.py b/kallithea/model/db.py
> --- a/kallithea/model/db.py
> +++ b/kallithea/model/db.py
> @@ -1389,6 +1389,16 @@
> grouped[cmt.revision].append(cmt)
> return grouped
>
> + def count_comments(self, comments):
> + """
> + Returns the count of (non-empty) comments for this repository
> grouped by revisions
>
>
> Perhaps clarify that the purpose is to filter out comments that just are
> carriers of a status change/vote?
Good point.
> +
> + :param comments: a {revision: comment} dict, as returned by
> get_comments()
> + """
> + def count_revision_comments(revision_comments):
> + return len([cm for cm in revision_comments if cm.text])
> + return {raw_id: count_revision_comments(revision_comments) for
> raw_id, revision_comments in comments.iteritems()}
>
>
> This is Python 2.7 syntax - we still support 2.6. (That could perhaps change
> - but that is a separate discussion.)
I can keep it compatible with 2.6 is that is the current policy.
> The local function seems a bit overkill. I would just make it:
>
> return dict((raw_id, len([cm for cm in revision_comments if
> cm.text]))
> for raw_id, revision_comments in comments.iteritems())
I added a function as a way to document what the code is doing. The
inline code feels more cryptic to me but I can do what you suggest if
that is what you prefer.
> +
> def statuses(self, revisions):
> """
> Returns statuses for this repository.
> diff --git a/kallithea/templates/changelog/changelog.html
> b/kallithea/templates/changelog/changelog.html
> --- a/kallithea/templates/changelog/changelog.html
> +++ b/kallithea/templates/changelog/changelog.html
> @@ -120,11 +120,11 @@
> <div class="log-container">
> <div class="message"
> id="C-${cs.raw_id}">${h.urlify_commit(cs.message,
> c.repo_name,h.url('changeset_home',repo_name=c.repo_name,revision=cs.raw_id))}</div>
> <div class="extra-container">
> - %if c.comments.get(cs.raw_id):
> + %if c.comment_counts.get(cs.raw_id, 0):
> <div class="comments-container">
> <div class="comments-cnt"
> title="${_('Changeset has comments')}">
> <a
> href="${c.comments[cs.raw_id][0].url()}">
> -
> ${len(c.comments[cs.raw_id])}
> +
> ${c.comment_counts.get(cs.raw_id, 0)}
>
>
> Here we know that the changeset is in comment_counts ... so why not just use
> []? (The same applies elsewhere.)
OK. I went for consistency and I think I saw it done that way
somewhere else, but I might be mistaken...
> <i
> class="icon-comment-discussion"></i>
> </a>
> </div>
> diff --git a/kallithea/templates/changelog/changelog_summary_data.html
> b/kallithea/templates/changelog/changelog_summary_data.html
> --- a/kallithea/templates/changelog/changelog_summary_data.html
> +++ b/kallithea/templates/changelog/changelog_summary_data.html
> @@ -31,11 +31,11 @@
> </div>
> </td>
> <td class="compact">
> - %if c.comments.get(cs.raw_id,[]):
> + %if c.comment_counts.get(cs.raw_id, 0):
> <div class="comments-container">
> <div title="${('comments')}">
> - <a href="${c.comments[cs.raw_id][0].url()}">
> - <i
> class="icon-comment"></i>${len(c.comments[cs.raw_id])}
> + <a href="${c.comments[cs.raw_id][0].url()}">
> + <i
> class="icon-comment"></i>${c.comment_counts.get(cs.raw_id, 0)}
> </a>
> </div>
> </div>
> diff --git a/kallithea/templates/compare/compare_cs.html
> b/kallithea/templates/compare/compare_cs.html
> --- a/kallithea/templates/compare/compare_cs.html
> +++ b/kallithea/templates/compare/compare_cs.html
> @@ -25,11 +25,11 @@
> <i class="icon-circle
> changeset-status-${c.statuses[cs.raw_id][0]}"></i>
> </div>
> %endif
> - %if c.cs_comments.get(cs.raw_id):
> + %if c.cs_comment_counts.get(cs.raw_id, 0):
> <div class="comments-container">
> <div class="comments-cnt" title="${_('Changeset has
> comments')}">
> <a href="${c.cs_comments[cs.raw_id][0].url()}">
> - ${len(c.cs_comments[cs.raw_id])}
> + ${c.cs_comment_counts.get(cs.raw_id, 0)}
> <i class="icon-comment"></i>
> </a>
> </div>
More information about the kallithea-general
mailing list