[PATCH] comments: avoid confusing 'No comments' on empty general comments
Thomas De Schampheleire
patrickdepinguin at gmail.com
Mon Apr 20 06:14:43 EDT 2015
On Sun, Apr 19, 2015 at 3:45 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 04/19/2015 06:05 AM, Thomas De Schampheleire wrote:
>> On Sun, Apr 19, 2015 at 12:37 AM, Mads Kiilerich <mads at kiilerich.com>
>>> On 04/18/2015 04:12 PM, Thomas De Schampheleire wrote:
>>>> # HG changeset patch
>>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>>> # Date 1429294302 -7200
>>>> # Fri Apr 17 20:11:42 2015 +0200
>>>> # Node ID e25ee448dae32e2e21f1b66763dae468f45c00f0
>>>> # Parent a7cc66bf51ec89134cb7b02aca044bb74a56f768
>>>> comments: avoid confusing 'No comments' on empty general comments
>>>> When a general comment (with or without status change) is added to a
>>>> or pull request, and no text was added, Kallithea automatically used 'No
>>>> comments' as text. This is confusing when the reviewer has added one or
>>>> inline comments.
>>> Ok, it is not really 'no comments' in that case. Still, the status change
>>> done without an explicit comment to that. We can perhaps be more spot on.
>>> Also, right now, each comment (inline or global) is added separately. I
>>> see how your change would make more sense together with Jan's
>> I haven't seen any further comments on these changes. Have you had the
>> time to look at them, any feedback?
> Not yet. They are non-trivial and still on the growing todo list ;-)
>>>> Instead, use '(status change only)'.
>>> It will show up as a comment so let's use proper casing and punctuation.
>>> about "Status change without comment." or "Status change." and later on
>>> "Status change with inline comments."?
>> I added () to try and distinguish a user comment from an automatically
>> generated text. I assume you don't think it's necessary to make such
> I don't think it was clear to the user that it was an automatic comment. It
> would perhaps be better to say that ... or to use square brackets.
[Automatic comment: status change]
[Automatic comment: closing]
There would be no need to indicate 'without comments' as it's obvious
there are none.
> Alternatively, store it without any text in the database and generate the
> text "on demand" in the templates and give it different markup ...
This is probably even better. The text we display in the template
would then be the one we conclude on above.
>> I'm fine with 'Status change without comment.' though.
>>>> In the case where a user added
>>>> a comment without text and without status change (for example when
>>>> pull requesh), use '(no comments added)'.
>>> There is no reason to add comments without text and without status change
>>> and without closing. If it is closing, how about "Closing without
>> Fine for 'Closing without comment.'.
>> While I agree that comments without text and without status change and
>> without closing are pointless, but it is currently possible to create
>> such comments so the code would need to cater for this case.
>> Alternatively, we could either detect such situation at client-side,
>> or at server side? I don't know if it's worth the trouble though. We
>> could also just ignore such comments and not add anything to the
>> database (without error).
> Ideally the UI should make it "hard" to make noop comments and the server
> side should reject/ignore them. One of them would also be fine. I think the
> main point is that we shouldn't optimize for that case at all.
I'll look at this, probably ignoring on the server-side.
To handle it on the client-side would mean also checking if there was
a status change, so a bit more involved in the current codebase.
More information about the kallithea-general