Current implementation is showing n-1 patch-set candidates. The most recent patch-set is removed from the list. The rationale is that it would make little sense to diff against.
However it might still be informative to show the latest patch-set in the list. Try instead an approach where n candidates are shown but where a message is issued if the latest patch-set is chosen by the user.
Niklas Eklund <niklas.eklund@posteo.net> writes:
> Current implementation is showing n-1 patch-set candidates. The most> recent patch-set is removed from the list. The rationale is that it> would make little sense to diff against.
That's a solid argument
> However it might still be informative to show the latest patch-set in> the list. Try instead an approach where n candidates are shown but> where a message is issued if the latest patch-set is chosen by the> user.
Good point. It will make sense to show the latest patch-set since it
shows informative metadata such as comment authors. It is useful to know
wether the user has commented on the current patch-set or not.
Apr 27, 2022 19:13:13 Niklas Eklund <niklas.eklund@posteo.net>:
> Niklas Eklund <niklas.eklund@posteo.net> writes:>>> Current implementation is showing n-1 patch-set candidates. The most>> recent patch-set is removed from the list. The rationale is that it>> would make little sense to diff against.>> That's a solid argument
Let's verify that replying with the FairMail application works better,
than the initial message which was composed with the K-9 client
>> However it might still be informative to show the latest patch-set in>> the list. Try instead an approach where n candidates are shown but>> where a message is issued if the latest patch-set is chosen by the>> user.>> Good point. It will make sense to show the latest patch-set since it> shows informative metadata such as comment authors. It is useful to > know> wether the user has commented on the current patch-set or not.
Niklas Eklund <niklas.eklund@posteo.net> writes:
> Apr 27, 2022 19:13:13 Niklas Eklund <niklas.eklund@posteo.net>:>>> Niklas Eklund <niklas.eklund@posteo.net> writes:>>>>> Current implementation is showing n-1 patch-set candidates. The most>>> recent patch-set is removed from the list. The rationale is that it>>> would make little sense to diff against.>>>> That's a solid argument>>>>> However it might still be informative to show the latest patch-set in>>> the list. Try instead an approach where n candidates are shown but>>> where a message is issued if the latest patch-set is chosen by the>>> user.>>>> Good point. It will make sense to show the latest patch-set since it>> shows informative metadata such as comment authors. It is useful to >> know>> wether the user has commented on the current patch-set or not.
Here is a patch that proposes a possible implementation.
Niklas Eklund <niklas.eklund@posteo.net> writes:
> Niklas Eklund <niklas.eklund@posteo.net> writes:>>> Apr 27, 2022 19:13:13 Niklas Eklund <niklas.eklund@posteo.net>:>>>>> Niklas Eklund <niklas.eklund@posteo.net> writes:>>>>>>> Current implementation is showing n-1 patch-set candidates. The most>>>> recent patch-set is removed from the list. The rationale is that it>>>> would make little sense to diff against.>>>>>> That's a solid argument>>>>>>> However it might still be informative to show the latest patch-set in>>>> the list. Try instead an approach where n candidates are shown but>>>> where a message is issued if the latest patch-set is chosen by the>>>> user.>>>>>> Good point. It will make sense to show the latest patch-set since it>>> shows informative metadata such as comment authors. It is useful to >>> know>>> wether the user has commented on the current patch-set or not.>> Here is a patch that proposes a possible implementation.>> From 9db22842087405d94815609fe9fd7600267d9b74 Mon Sep 17 00:00:00 2001> From: Niklas Eklund <niklas.eklund@posteo.net>> Date: Wed, 27 Apr 2022 20:20:19 +0200> Subject: [PATCH] Update selection in partial review>> ---> egerrit.el | 14 ++++++++------> 1 file changed, 8 insertions(+), 6 deletions(-)>> diff --git a/egerrit.el b/egerrit.el> index e5a1812..9d4f890 100644> --- a/egerrit.el> +++ b/egerrit.el> @@ -1564,8 +1564,7 @@ This function will work both for review/feedback comments."> (let* ((egerrit--current-change (egerrit-get-detailed-change change))> (old-revision> (egerrit-select-revision egerrit--current-change> - (butlast> - (egerrit--change-revisions egerrit--current-change))))> + (egerrit--change-revisions egerrit--current-change)))
Removing butlast should do the trick, let's test this out.
> (new-revision> (if current-prefix-arg> (egerrit-select-revision egerrit--current-change> @@ -1573,10 +1572,13 @@ This function will work both for review/feedback comments."> (seq-filter (lambda (it) (> (egerrit--revision-number it)> (egerrit--revision-number old-revision))))))> (seq-first (seq-reverse (egerrit--change-revisions egerrit--current-change))))))> - (if (and (eq egerrit--mode 'reviewer)> - (not (egerrit--clean-git-repository change)))> - (message "The git repository can't be dirty when engaging in a review")> - (egerrit--open-partial-revision-diff old-revision new-revision egerrit--current-change)))))> + (if (= (egerrit--revision-number old-revision)> + (egerrit--revision-number new-revision))> + (message "Revisions are the same, no difference")
It's good that we add a message to the user to why a partial review
can't be generated given the patch-sets that the user has selected.
> + (if (and (eq egerrit--mode 'reviewer)> + (not (egerrit--clean-git-repository change)))> + (message "The git repository can't be dirty when engaging in a review")> + (egerrit--open-partial-revision-diff old-revision new-revision egerrit--current-change))))))> > ;;;###autoload> (defun egerrit-publish-comments ()> -- > 2.34.0
Will try out this implementation.