search

joshcho / ChatGPT.el

ChatGPT in Emacs
GNU General Public License v3.0
396 stars 34 forks source link

Don't rely on GPT too much. #1

Closed progfolio closed 1 year ago

progfolio commented 1 year ago

This package will fail to install due to it missing a package header.

Without it there's no way for the package manager to know which dependencies are needed or what minimum version of Emacs the package supports.

chatgpt-repo-path should not be hardcoded to assume straight.el was used to install the package. It would be better to dynamically detect the repository path.

checkdoc shows the following warnings/errors:

chatgpt.el                    1   0 note     e-f-c    The first line should be of the form: ";;; package --- Summary"
chatgpt.el                    1   0 note     e-f-c    You should have a section marked ";;; Commentary:"
chatgpt.el                    1   1 error    e-f-b-c  Cannot open load file: No such file or directory, epc
chatgpt.el                    4   1 note     e-f-c    You should have a section marked ";;; Code:"
chatgpt.el                   20   2 note     e-f-c    First line is not a complete sentence
chatgpt.el                   26   2 note     e-f-c    First line is not a complete sentence
chatgpt.el                   31   2 note     e-f-c    First line is not a complete sentence
chatgpt.el                   53  38 note     e-f-c    Lisp symbol ‘chatgpt-query’ should appear in quotes
chatgpt.el                   80   2 note     e-f-c    First line is not a complete sentence
chatgpt.el                   95   2 note     e-f-c    All variables and subroutines might as well have a documentation string
chatgpt.el                  100   2 note     e-f-c    All variables and subroutines might as well have a documentation string
chatgpt.el                  115   0 note     e-f-c    Lisp symbol ‘chatgpt-query’ should appear in quotes
chatgpt.el                  176   0 note     e-f-c    Open parenthesis in column 0 should be escaped
chatgpt.el                  183 114 note     e-f-c    Error messages should *not* end with a period
chatgpt.el                  187   2 note     e-f-c    First line is not a complete sentence

Instead of relying on epc and deffered, you may be able to redesign this as a major mode derived from comint-mode.

See: https://www.masteringemacs.org/article/comint-writing-command-interpreter

The chatgpt-display command is missing an interactive spec. It's implementation could likely be replaced by Emacs built-in pop-to-buffer function, e.g.:

(save-selected-window
  (pop-to-buffer (get-buffer-create "*test*")))

However, it would be better to let the user decide which window ends up selected. pop-to-buffer will obey display-buffer-alist on its own.

There are autoload cookies for private elisp functions. By convention these shouldn't be called by anyone consuming the library, so they should not be autoloaded.

chatpt--newline-twice and chatgpt--delete-line seem superfluous.

I'd recommend re-implementing the python script in elisp to avoid the dependency on python altogether.

joshcho commented 1 year ago

Thank you for the recommendations. Your points about package header, checkdoc, pop-to-buffer, autoload, and chatgpt--newline-twice and chatgpt--delete-line should be addressed in a9734dda7d83fc2a406bec541d9e4ec6f5ac3f77.

I will look into comint-mode.

Please feel free to submit any issues and pull requests. I am a novice in Emacs package development, and any feedback is welcome.