[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[BUG] comint-strip-ctrl-m doesn't function as documentation states
From: |
Jonathan |
Subject: |
[BUG] comint-strip-ctrl-m doesn't function as documentation states |
Date: |
Tue, 19 Mar 2024 19:55:55 +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))=.
#+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.
- [BUG] comint-strip-ctrl-m doesn't function as documentation states,
Jonathan <=