bug#29842: 27.0; `dired-change-marks': args should not be optional

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Drew Adams]

1. There is no use of this function in the Emacs Lisp sources that needs
   to have the args be optional.

2. If (dired-change-marks) is evaluated then you get an error:

   Debugger entered--Lisp error: (error "Format specifier doesn't match
                                         argument type") 
    format("\n%c" "nil")
    dired-change-marks()
    eval((dired-change-marks))

This bug is apparently quite old - it is present in Emacs 20, for
example.

In GNU Emacs 27.0.50 (build 4, x86_64-w64-mingw32)
 of 2017-12-21
Repository revision: b1cf262a79463f28164ea1c2ffee3c657ce02ea4
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install -C 'CFLAGS=-O2 -static -g3'
 host_alias=x86_64-w64-mingw32 PKG_CONFIG_PATH=/mingw64/lib/pkgconfig'

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Drew Adams]

Furthermore, the code should perhaps not just call
`ding' if you hit RET.  Instead of this:

 (if (or (eq old ?\r) (eq new ?\r))
     (ding)
  ...)

it should perhaps do something like this:

 (unless (char-displayable-p old)
   (error "Not a displayable character: `%c'" old))
 (unless (char-displayable-p new)
   (error "Not a displayable character: `%c'" new))

The current code predates the existence of
`char-displayable-p'.

(Yes, that's a different bug, but perhaps it can be
taken care of in the context of this report.  If not,
don't worry about it.)

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Tino Calancha]

Drew Adams <drew.adams@oracle.com> writes:

> 1. There is no use of this function in the Emacs Lisp sources that needs
>    to have the args be optional.
Yes, it has sense that the arguments be mandatory.

> 2. If (dired-change-marks) is evaluated then you get an error:
>
>    Debugger entered--Lisp error: (error "Format specifier doesn't match
>                                          argument type") 
>     format("\n%c" "nil")
>     dired-change-marks()
>     eval((dired-change-marks))
Then the programmer would get the more intuitive
'Wrong number of arguments' error.

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Tino Calancha]

Drew Adams <drew.adams@oracle.com> writes:

> Furthermore, the code should perhaps not just call
> `ding' if you hit RET.  Instead of this:
>
>  (if (or (eq old ?\r) (eq new ?\r))
>      (ding)
>   ...)
>
> it should perhaps do something like this:
>
>  (unless (char-displayable-p old)
>    (error "Not a displayable character: `%c'" old))
>  (unless (char-displayable-p new)
>    (error "Not a displayable character: `%c'" new))
>
> The current code predates the existence of
> `char-displayable-p'.
The more information the better; just a ding force you to read the
source code to know what was wrong.
Maybe `user-error' instead of `error'?

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Drew Adams]

> The more information the better; just a ding force you to read the
> source code to know what was wrong.
> Maybe `user-error' instead of `error'?

Yes `user-error' is appropriate here.  Sorry, I always
forget about its existence.

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

> 1. There is no use of this function in the Emacs Lisp sources that needs
>    to have the args be optional.
>
> 2. If (dired-change-marks) is evaluated then you get an error:
>
>    Debugger entered--Lisp error: (error "Format specifier doesn't match
>                                          argument type") 
>     format("\n%c" "nil")
>     dired-change-marks()
>     eval((dired-change-marks))
>
> This bug is apparently quite old - it is present in Emacs 20, for
> example.

Fixed on master in commit 3d81d2326a.

Best regards,
Stefan Kangas

bug#29842: 27.0; `dired-change-marks': args should not be optional

[Stefan Kangas]

close 29842 28.1
thanks

Drew Adams <drew.adams@oracle.com> writes:

> Furthermore, the code should perhaps not just call
> `ding' if you hit RET.  Instead of this:
>
>  (if (or (eq old ?\r) (eq new ?\r))
>      (ding)
>   ...)
>
> it should perhaps do something like this:
>
>  (unless (char-displayable-p old)
>    (error "Not a displayable character: `%c'" old))
>  (unless (char-displayable-p new)
>    (error "Not a displayable character: `%c'" new))
>
> The current code predates the existence of
> `char-displayable-p'.
>
> (Yes, that's a different bug, but perhaps it can be
> taken care of in the context of this report.  If not,
> don't worry about it.)

Fixed on master in commit e5e31aab9b.

Best regards,
Stefan Kangas