Re: [PATCHES] Keyword args for file openers; coding scan off by default

[Ludovic Courtès]

Mark H Weaver <address@hidden> skribis:

> From 951b9d224d84bfec271b51615bc095013d153694 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <address@hidden>
> Date: Sat, 6 Apr 2013 23:19:55 -0400
> Subject: [PATCH 3/3] Add keyword arguments to file opening procedures.
>
> * libguile/fports.c (scm_open_file_with_encoding): New API function,
>   containing the code previously found in 'scm_open_file', but modified
>   to accept the new 'guess_encoding' and 'encoding' arguments.
>
>   (scm_open_file): Now just a simple wrapper that calls
>   'scm_open_file_with_encoding'.
>
>   (scm_i_open_file): New implementation of 'open-file' that accepts
>   keyword arguments '#:guess-encoding' and '#:encoding', and calls
>   'scm_open_file_with_encoding'.
>
>   (scm_init_fports_keywords): New initialization function that gets
>   called after keywords are initialized.
>
> * libguile/fports.h (scm_open_file_with_encoding,
>   scm_init_fports_keywords): Add prototypes.
>
> * libguile/init.c (scm_i_init_guile): Call 'scm_init_fports_keywords'.
>
> * module/ice-9/boot-9.scm: Add enhanced versions of 'open-input-file',
>   'open-output-file', 'call-with-input-file', 'call-with-output-file',
>   'with-input-from-file', 'with-output-to-file', and
>   'with-error-to-file', that accept keyword arguments '#:binary',
>   '#:encoding', and (for input port constructors) '#:guess-encoding'.
>
> * doc/ref/api-io.texi (File Ports): Update documentation.
>
> * test-suite/tests/ports.test ("keyword arguments for file openers"):
>   Add tests.

Looks good.

Minor comments:

> address@hidden {Scheme Procedure} open-file filename mode @
> +                          [#:guess-encoding=#f] [#:encoding=#f]
> address@hidden {C Function} scm_open_file_with_encoding @
> +                     (filename, mode, guess_encoding, encoding)
>  @deffnx {C Function} scm_open_file (filename, mode)
>  Open the file whose name is @var{filename}, and return a port
>  representing that file.  The attributes of the port are
> @@ -900,8 +903,17 @@ to the underlying @code{open} call.  Still, the flag is 
> generally useful
>  because of its port encoding ramifications.
>  @end table
>  
> -If a file cannot be opened with the access
> -requested, @code{open-file} throws an exception.
> +Unless binary mode is requested, the character encoding of the new port
> +is determined as follows: First, if @var{guess-encoding} is true,
> +heuristics will be used to guess the encoding of the file.  If it is

“heuristics” is vague.  I’d prefer “the ‘file-encoding’ procedure is
called to check for Emacs-style coding declarations (@pxref{Character
Encoding of Source Files})”.  Should BOMs also be mentioned?

> +/* scm_open_file_with_encoding
>   * Return a new port open on a given file.
>   *
> + * Use heuristics to guess the encoding is GUESS_ENCODING
> + * is true, else use ENCODING if not false, else use the
> + * default port encoding.

Likewise.

And you’re welcome to remove the leading stars also.  :-)

> +SCM k_guess_encoding = SCM_UNDEFINED;
> +SCM k_encoding       = SCM_UNDEFINED;

Add ‘static’.

> +  scm_c_bind_keyword_arguments (FUNC_NAME, keyword_args, 0,
> +                                k_guess_encoding, &guess_encoding,
> +                                k_encoding, &encoding,
> +                                SCM_UNDEFINED);

Comes in handy.  ;-)

> +(define* (open-input-file
> +          str #:key (binary #f) (encoding #f) (guess-encoding #f))
> +  "Takes a string naming an existing file and returns an input port
> +capable of delivering characters from the file.  If the file
> +cannot be opened, an error is signalled."

It’s a detail, for these procedures, I would s/str/file/, and in
docstrings s/file STR/FILE/.

The test suite looks comprehensive, that’s great.

Thanks!

Ludo’.