search

emacs-openai / openai

Elisp library for the OpenAI API
GNU General Public License v3.0
100 stars 17 forks source link

Suggestions #5

Open Fuco1 opened 1 year ago

Fuco1 commented 1 year ago

I have some suggestions for the generic API package. They are a bit drastic and depart from how this package is organized, but here it goes.

  1. Remove the defcustoms. The purpose of a generic API module is to expose all the functionality through request interfaces and some generic submit function.
  2. Expose the responses as classes or structures since they are known. That will make working with results much simpler.
  3. openai-key should not be defcustom. Many users commit their custom file and they could accidentally commit their tokens.
  4. Merge all the files into one file. They are too small to make much sense.
  5. Expose all the constants as named constants

For inspiration, see https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-protocol.el

Since the openai API is small, things can be hand-written and not generated with complicated macros, unlike LSP which has hundreds of interfaces.

After we discuss this, I'll be happy to start working on it and provide some pull requests.

jcs090218 commented 1 year ago
  1. Remove the defcustoms. The purpose of a generic API module is to expose all the functionality through request interfaces and some generic submit function.

I agree. I think the best example is like posframe using the macro cl-defun and expose all of them.

  1. Expose the responses as classes or structures since they are known. That will make working with results much simpler.

I don't have enough experience regarding OOP in elisp, but I am sure this is helpful. I would need to take some times to learn it. 😅

  1. openai-key should not be defcustom. Many users commit their custom file and they could accidentally commit their tokens.

What's your suggestion to this? I use auth-source to store my key.

  1. Merge all the files into one file. They are too small to make much sense.

I think it's great to organize this way, so I can find the API I need depending on the type of it. 🤔

For inspiration, see https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-protocol.el

Since the openai API is small, things can be hand-written and not generated with complicated macros, unlike LSP which has hundreds of interfaces.

👍

Fuco1 commented 1 year ago
  1. Yea, for example with keys is fine, and it's discoverable and can be documented in the docstring.
  2. Doesn't need to be EIEIO, we can also use cl-defstruct or something else, the idea is that it would be nice to have some documentation.
  3. We could read from environment variable or just make it a defvar, so people can setq it in some personal file. For example, I have personal.el in my config which I do not commit and that's where I set api keys and so on.
  4. Actually, in elisp people tend to have one huge file over many small ones for some historical reasons, but now that I think about it, using Elsa + LSP would be much nicer on many smaller files because they can each be re-analyzed separately. I need to also switch over to this style. It's a new world :D
jcs090218 commented 1 year ago
  1. Yea, for example with keys is fine, and it's discoverable and can be documented in the docstring.

Let me know if you have more thoughts on this! cl-defun is what I am more familiar with, so I propose it. But would be great to share and exchange ideas!

  1. Doesn't need to be EIEIO, we can also use cl-defstruct or something else, the idea is that it would be nice to have some documentation.

Sounds good to me! ;)

  1. We could read from environment variable or just make it a defvar, so people can setq it in some personal file. For example, I have personal.el in my config which I do not commit and that's where I set api keys and so on.

Oh, okay. So we can just use defvar here? That's cool. :D

  1. Actually, in elisp people tend to have one huge file over many small ones for some historical reasons, but now that I think about it, using Elsa + LSP would be much nicer on many smaller files because they can each be re-analyzed separately. I need to also switch over to this style. It's a new world :D

Yeah, I understand one huge file's benefits, but I still decided to do it this way. Let's see what happens next!? If each file becomes smaller after refactoring, and does not fit the "multi-file" structure, we can always switch back to one huge file. ;)

jcs090218 commented 1 year ago

I've made an adjustment in #7 and exposed all parameters! Let me know if you have more suggestions! :D