[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