[BUG] comint-strip-ctrl-m doesn't function as documentation states

[BUG] comint-strip-ctrl-m doesn't function as documentation states

[Jonathan]

Hey folks,

There appears to either be a bug or just inaccurate documentation of =comint-strip-ctrl-m=. At the very bottom, I've included some context about my use case by which I discovered this bug that may or may not be relevant to you. The documentation for that function states:

#+begin_quote
Strip trailing ^M characters from the current output group.

This function could be on comint-output-filter-functions or bound to a key.
#+end_quote

=comint-output-filter-functions= states the following:

#+begin_quote
...These functions get one argument, a string containing the text as originally
inserted.  Note that this might not be the same as the buffer contents between
comint-last-output-start and the buffer's process-mark, if other filter
functions have already modified the buffer.
#+end_quote

Looking at the implementation of =comint-strip-ctrl-m= it appears that it completely ignores the =string= argument and instead uses =(get-buffer-process (current-buffer))=.

#+begin_src emacs-lisp
(defun comint-strip-ctrl-m (&optional _string interactive)
  "Strip trailing `^M' characters from the current output group.
This function could be on `comint-output-filter-functions' or bound to a key."
  (interactive (list nil t))
  (let ((process (get-buffer-process (current-buffer))))
    (if (not process)
        ;; This function may be used in
        ;; `comint-output-filter-functions', and in that case, if
        ;; there's no process, then we should do nothing.  If
        ;; interactive, report an error.
        (when interactive
          (error "No process in the current buffer"))
      ;;; rest omitted for brefity
      )))
#+end_src

This represents unexpected and undocumented behavior, as you anticipate =comint-strip-ctrl-m= to behave like any other comint output filter functions. I'd like to propose 3 different possible solutions for a patch and would like input on which is preferred as this code was originally introduced in 1994. I can submit a patch once a solution has been determined.

1. Update the documentation and leave as is. This is the simplest solution and would just require doc-string updates to indicate that =comint-strip-ctrl-m= is a "unique" filter function among the other filter functions that exist. This does not seem preferable to me.

2. Update the implementation of =comint-strip-ctrl-m= itself to conform it to the documented API. This would mean anything currently depending on it reading the =current-buffer= would break, and since there are plenty of unknowns in that regard, this also does not seem preferable.

3. Add a new version of the function with a different name that conforms to the documented API =comint-strip-ctrl-m-output= or something similar and deprecate the original.

If we do decide to deprecate the original, I'm happy to include a deprecation warning and keep an eye on it popping up in core to ensure that we handle those issues over time.

Any guidance would be useful. Thank you all for you're hard work.

- Jonathan

PS: Additional Context as promised:

I was developing a package that runs SQL queries in a "hidden" SQLi buffer and so I needed to strip carriage return characters out of the output. Using this filter I had thought it would perform the task, but it did not. So digging through the documentation I discovered this error. I think it's pretty reasonable that filter functions conform to the documented api or should at least be noted otherwise.

Re: [BUG] comint-strip-ctrl-m doesn't function as documentation states

[Po Lu]

Jonathan <public@jds.work> writes:

> Hey folks,
>
> There appears to either be a bug or just inaccurate documentation of
> =comint-strip-ctrl-m=. At the very bottom, I've included some context
> about my use case by which I discovered this bug that may or may not
> be relevant to you. The documentation for that function states:
>
> #+begin_quote
> Strip trailing ^M characters from the current output group.
>
> This function could be on comint-output-filter-functions or bound to a key.
> #+end_quote
>
>
> =comint-output-filter-functions= states the following:
>
> #+begin_quote
> ...These functions get one argument, a string containing the text as originally
> inserted.  Note that this might not be the same as the buffer contents between
> comint-last-output-start and the buffer's process-mark, if other filter
> functions have already modified the buffer.
> #+end_quote
>
>
> Looking at the implementation of =comint-strip-ctrl-m= it appears that
> it completely ignores the =string= argument and instead uses
> =(get-buffer-process (current-buffer))=.
>
> #+begin_src emacs-lisp
> (defun comint-strip-ctrl-m (&optional _string interactive)
>   "Strip trailing `^M' characters from the current output group.
> This function could be on `comint-output-filter-functions' or bound to a key."
>   (interactive (list nil t))
>   (let ((process (get-buffer-process (current-buffer))))
>     (if (not process)
>         ;; This function may be used in
>         ;; `comint-output-filter-functions', and in that case, if
>         ;; there's no process, then we should do nothing.  If
>         ;; interactive, report an error.
>         (when interactive
>           (error "No process in the current buffer"))
>       ;;; rest omitted for brefity
>       )))
> #+end_src
>
> This represents unexpected and undocumented behavior, as you
> anticipate =comint-strip-ctrl-m= to behave like any other comint
> output filter functions. I'd like to propose 3 different possible
> solutions for a patch and would like input on which is preferred as
> this code was originally introduced in 1994. I can submit a patch once
> a solution has been determined.
>
> 1. Update the documentation and leave as is. This is the simplest
> solution and would just require doc-string updates to indicate that
> =comint-strip-ctrl-m= is a "unique" filter function among the other
> filter functions that exist. This does not seem preferable to me.
>
> 2. Update the implementation of =comint-strip-ctrl-m= itself to
> conform it to the documented API. This would mean anything currently
> depending on it reading the =current-buffer= would break, and since
> there are plenty of unknowns in that regard, this also does not seem
> preferable.
>
> 3. Add a new version of the function with a different name that
> conforms to the documented API =comint-strip-ctrl-m-output= or
> something similar and deprecate the original.
>
> If we do decide to deprecate the original, I'm happy to include a
> deprecation warning and keep an eye on it popping up in core to ensure
> that we handle those issues over time.
>
> Any guidance would be useful. Thank you all for you're hard work.
>
> - Jonathan
>
> PS: Additional Context as promised:
>
> I was developing a package that runs SQL queries in a "hidden" SQLi
> buffer and so I needed to strip carriage return characters out of the
> output. Using this filter I had thought it would perform the task, but
> it did not. So digging through the documentation I discovered this
> error. I think it's pretty reasonable that filter functions conform to
> the documented api or should at least be noted otherwise.

Thanks.  Please direct bug reports to bug-gnu-emacs@gnu.org that the bug
tracker may record its progress: this list is for discussion that
directly concerns development only.

Re: [BUG] comint-strip-ctrl-m doesn't function as documentation states

[Jonathan]

[-- Attachment #1: Type: text/plain, Size: 4424 bytes --]

Apologies for the misleading subject. I had assumed that discussion of an approach about that particular filter function would happen here as technically it’s not a bug but inaccurate behavior by way of the documentation. I’m unsure if someone knows whether or not it’s “inaccuracy” is intentional and whether there’s a preferred approach to fixing.

If this isn’t the place for that discussion I can post in the bug-gnu-emacs mailing list.

- Jonathan

On Tue, Mar 19, 2024 at 10:29 PM, Po Lu <[luangruo@yahoo.com](mailto:On Tue, Mar 19, 2024 at 10:29 PM, Po Lu <<a href=)> wrote:

> Jonathan <public@jds.work> writes:
>
>> Hey folks,
>>
>> There appears to either be a bug or just inaccurate documentation of
>> =comint-strip-ctrl-m=. At the very bottom, I've included some context
>> about my use case by which I discovered this bug that may or may not
>> be relevant to you. The documentation for that function states:
>>
>> #+begin_quote
>> Strip trailing ^M characters from the current output group.
>>
>> This function could be on comint-output-filter-functions or bound to a key.
>> #+end_quote
>>
>>
>> =comint-output-filter-functions= states the following:
>>
>> #+begin_quote
>> ...These functions get one argument, a string containing the text as originally
>> inserted. Note that this might not be the same as the buffer contents between
>> comint-last-output-start and the buffer's process-mark, if other filter
>> functions have already modified the buffer.
>> #+end_quote
>>
>>
>> Looking at the implementation of =comint-strip-ctrl-m= it appears that
>> it completely ignores the =string= argument and instead uses
>> =(get-buffer-process (current-buffer))=.
>>
>> #+begin_src emacs-lisp
>> (defun comint-strip-ctrl-m (&optional _string interactive)
>> "Strip trailing `^M' characters from the current output group.
>> This function could be on `comint-output-filter-functions' or bound to a key."
>> (interactive (list nil t))
>> (let ((process (get-buffer-process (current-buffer))))
>> (if (not process)
>> ;; This function may be used in
>> ;; `comint-output-filter-functions', and in that case, if
>> ;; there's no process, then we should do nothing. If
>> ;; interactive, report an error.
>> (when interactive
>> (error "No process in the current buffer"))
>> ;;; rest omitted for brefity
>> )))
>> #+end_src
>>
>> This represents unexpected and undocumented behavior, as you
>> anticipate =comint-strip-ctrl-m= to behave like any other comint
>> output filter functions. I'd like to propose 3 different possible
>> solutions for a patch and would like input on which is preferred as
>> this code was originally introduced in 1994. I can submit a patch once
>> a solution has been determined.
>>
>> 1. Update the documentation and leave as is. This is the simplest
>> solution and would just require doc-string updates to indicate that
>> =comint-strip-ctrl-m= is a "unique" filter function among the other
>> filter functions that exist. This does not seem preferable to me.
>>
>> 2. Update the implementation of =comint-strip-ctrl-m= itself to
>> conform it to the documented API. This would mean anything currently
>> depending on it reading the =current-buffer= would break, and since
>> there are plenty of unknowns in that regard, this also does not seem
>> preferable.
>>
>> 3. Add a new version of the function with a different name that
>> conforms to the documented API =comint-strip-ctrl-m-output= or
>> something similar and deprecate the original.
>>
>> If we do decide to deprecate the original, I'm happy to include a
>> deprecation warning and keep an eye on it popping up in core to ensure
>> that we handle those issues over time.
>>
>> Any guidance would be useful. Thank you all for you're hard work.
>>
>> - Jonathan
>>
>> PS: Additional Context as promised:
>>
>> I was developing a package that runs SQL queries in a "hidden" SQLi
>> buffer and so I needed to strip carriage return characters out of the
>> output. Using this filter I had thought it would perform the task, but
>> it did not. So digging through the documentation I discovered this
>> error. I think it's pretty reasonable that filter functions conform to
>> the documented api or should at least be noted otherwise.
>
> Thanks. Please direct bug reports to bug-gnu-emacs@gnu.org that the bug
> tracker may record its progress: this list is for discussion that
> directly concerns development only.

[-- Attachment #2: Type: text/html, Size: 5239 bytes --]

Re: [BUG] comint-strip-ctrl-m doesn't function as documentation states

[Po Lu]

Jonathan <public@jds.work> writes:

> Apologies for the misleading subject. I had assumed that discussion of
> an approach about that particular filter function would happen here as
> technically it’s not a bug but inaccurate behavior by way of the
> documentation. I’m unsure if someone knows whether or not it’s
> “inaccuracy” is intentional and whether there’s a preferred approach
> to fixing.
>
> If this isn’t the place for that discussion I can post in the
> bug-gnu-emacs mailing list.

Our documentation's being unclear is a bug in its own right, so please
don't hesitate to report this as such.

Re: [BUG] comint-strip-ctrl-m doesn't function as documentation states

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Wed, 20 Mar 2024 11:29:12 +0800
> 
> Thanks.  Please direct bug reports to bug-gnu-emacs@gnu.org that the bug
> tracker may record its progress: this list is for discussion that
> directly concerns development only.

Indeed, reporting to bug-gnu-emacs is better.