bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#69914: comint-strip-ctrl-m doesn't function as documentation states


From: Jonathan
Subject: bug#69914: comint-strip-ctrl-m doesn't function as documentation states
Date: Wed, 20 Mar 2024 14:15:39 +0000

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))= in direct contradiction to the documentation.

#+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 brevity
      )))
#+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 your 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.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]