darcs

Patch 1892 rename actually_get_log to get_log_using... (and 1 more)

Title rename actually_get_log to get_log_using... (and 1 more)
Superseder Nosy List bf
Related Issues
Status accepted Assigned To bf
Milestone

Created on 2019-08-26.09:48:40 by bf, last changed 2019-09-04.08:55:45 by ganesh.

Files
File name Status Uploaded Type Edit Remove
factor-out-common-code-of-check_badname-and-prompt_patchname.dpatch bf, 2019-08-31.01:07:16 application/x-darcs-patch
patch-preview.txt bf, 2019-08-26.09:48:39 text/x-darcs-patch
patch-preview.txt bf, 2019-08-31.01:07:16 text/x-darcs-patch
rename-actually_get_log-to-get_log_using_editor.dpatch bf, 2019-08-26.09:48:39 application/x-darcs-patch
unnamed bf, 2019-08-26.09:48:39 text/plain
unnamed bf, 2019-08-31.01:07:16 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21201 (view) Author: bf Date: 2019-08-26.09:48:39
I stumbled over these issues (not really bugs) when I tested what happens if
we try to "fake" a tag. Fortunately this is handled by the UI layer, but the
way it is handled should be improved IMO.

2 patches for repository http://darcs.net/screened:

patch 88b3d7ced3ddbb81565ce039a33a410dc1c46f38
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 15:03:14 CEST 2019
  * rename actually_get_log to get_log_using_editor

patch 238ffe4bb9aa170249bc359c64d81355ff5b8a82
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 14:01:09 CEST 2019
  * fix prompting when we get a bad patch name
  
  If the user gives a patch name on the command line and that turns out to be
  bad, we want darcs to fail and not prompt the user. Think of using 'darcs
  record' in a script...
  Also, if we do prompt for a patch name and get a bad name, we should explain
  why it is bad before prompting again.
  Finally, when we get a bad name from reading a log file or from launching an
  editor, we also want to fail, rather than prompting the user.
Attachments
msg21222 (view) Author: bf Date: 2019-08-26.20:51:54
Not screening yet as this is a change in behavior.
msg21256 (view) Author: bf Date: 2019-08-28.09:05:31
Ganesh, could you take a quick look at the patch description and tell me
if you agree with the UI change? Detailed review can wait for later but
I would like to screen this.
msg21257 (view) Author: ganesh Date: 2019-08-28.09:18:12
I'm fine with the UI change.
msg21289 (view) Author: ganesh Date: 2019-08-30.22:22:40
>  * fix prompting when we get a bad patch name

Fine

>  * fix prompting when we get a bad patch name

The duplication between check_badname and prompt_patchname could be
factored out. We just need is_badname :: String -> Maybe String.

Rest looks fine.
msg21291 (view) Author: bf Date: 2019-08-31.01:07:16
Following up on review. Note sure if this is strictly better. The
duplication is removed but the code is a perhaps harder to read now.

1 patch for repository http://darcs.net/screened:

patch 83ac6de5ab032ae7b1a418535efcd57b4c464b60
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 31 03:02:24 CEST 2019
  * factor out common code of check_badname and prompt_patchname
Attachments
msg21298 (view) Author: ganesh Date: 2019-08-31.09:09:49
> Following up on review. Note sure if this is strictly better. The
> duplication is removed but the code is a perhaps harder to read now.

I think if it is harder to read, it's because of the use of 'maybe'
rather than an explicit case.

If you changed

  check_badname n =
    if null n then
      fail "The patch name must not be empty!"
    else if "TAG " `isPrefixOf` n then
      fail "The patch name must not start with \"TAG \"!"
    else
      return ()

into

  check_badname n =
    case just_a_badname n of
      Just err -> fail err
      Nothing -> return ()

Then I think it'd be very clear.

  check_badname = maybe (return ()) fail . just_a_badname

is obviously equivalent when we think about it, but perhaps a bit harder
to immediately follow. (But either is fine with me)
msg21354 (view) Author: bf Date: 2019-09-03.18:18:53
I have just screened my follow-up patch as is.
History
Date User Action Args
2019-08-26 09:48:40bfcreate
2019-08-26 20:51:54bfsetmessages: + msg21222
2019-08-28 09:05:31bfsetmessages: + msg21256
2019-08-28 09:18:12ganeshsetmessages: + msg21257
2019-08-29 12:48:24bfsetstatus: needs-screening -> needs-review
2019-08-30 22:22:40ganeshsetstatus: needs-review -> followup-requested
assignedto: bf
messages: + msg21289
2019-08-31 01:07:16bfsetfiles: + patch-preview.txt, factor-out-common-code-of-check_badname-and-prompt_patchname.dpatch, unnamed
messages: + msg21291
2019-08-31 08:46:01ganeshsetstatus: followup-requested -> review-in-progress
2019-08-31 09:09:49ganeshsetmessages: + msg21298
2019-09-03 18:18:53bfsetmessages: + msg21354
2019-09-03 18:24:21ganeshsetstatus: review-in-progress -> accepted-pending-tests
2019-09-04 08:55:45ganeshsetstatus: accepted-pending-tests -> accepted