[PATCH V3] summary, changelog, compare: make the comments counts more accurate
Mads Kiilerich
mads at kiilerich.com
Mon Feb 29 15:28:00 UTC 2016
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.)
>
>
> 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?
> +
> + :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.)
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())
> +
> 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.)
> <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>
>
>
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20160229/f915d34f/attachment-0001.html>
More information about the kallithea-general
mailing list