search

gptlint / gptlint

A linter with superpowers! 🔥 Use LLMs to enforce best practices across your codebase.
https://gptlint.dev
MIT License
213 stars 19 forks source link

add ability to supply custom dotenv config #15

Closed mergebandit closed 3 months ago

mergebandit commented 3 months ago

This PR adds support for a custom .env config path to pass to dotenv.config.

Since I want this to run before everything else, I didn't import the resolved-cli-config, but wasn't sure if there was a cleaner way to do this than to simply grab the args directly.

Here's the output of --help

image

and here's the output of --print-config for tsx bin/gptlint.ts --dotenv-path .env.local --print-config

image
vercel[bot] commented 3 months ago

@mergebandit is attempting to deploy a commit to the Saasify Team on Vercel.

To accomplish this, @mergebandit needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

transitive-bullshit commented 3 months ago

Hmmm I feel like we'd rather just have users export OPEN_API_KEY='...' in this case? tbh I'm not sure using dotenv for this type of CLI is something we should be doing in the first place; might be kind of insecure.

Thoughts?

mergebandit commented 3 months ago

Hmmm I feel like we'd rather just have users export OPEN_API_KEY='...' in this case? tbh I'm not sure using dotenv for this type of CLI is something we should be doing in the first place; might be kind of insecure.

Thoughts?

Yeah I wondered about that - but it wasn’t clear to me if you created the .env out of convenience or if you had expectations for more use cases.

I think it would be annoying to have to either create a .env to use gpt lint or have 5+ exports just to run it.

mergebandit commented 3 months ago

But then I also thought about how annoying it will be to have 824628282628726 cli flags 🤷‍♂️

transitive-bullshit commented 3 months ago

This is definitely a problem that needs addressing, I'm just not sure the best solution.

I was previously using .env as a dev convenience, and I'm a bit worried that users who have other secrets in a local project's .env won't want those potentially leaked by gptlint or the LLM side of things. On the flip side, even though we currently only use 1 env var, in the future, we may support more (see #6 for example), so .env would be nice to have. Also, other build CLIs do sometimes parse .env like next.js, though I feel like I'm conflicted on whether it's appropriate for gptlint.

Here are two potential solutions:

  1. we remove support for .env. currently it's only used for 1 env var, and you can pass it as export OPENAI_API_KEY or via --apiKey or via the config's llmOptions.apiKey. we could even punt it to users to use dotenv in their gptlint.config.js as opposed to having gptlint do it automatically.
  2. or we keep support for .env and merge this PR

@mergebandit thoughts?

mergebandit commented 3 months ago

This is definitely a problem that needs addressing, I'm just not sure the best solution.

I was previously using .env as a dev convenience, and I'm a bit worried that users who have other secrets in a local project's .env won't want those potentially leaked by gptlint or the LLM side of things. On the flip side, even though we currently only use 1 env var, in the future, we may support more (see #6 for example), so .env would be nice to have. Also, other build CLIs do sometimes parse .env like next.js, though I feel like I'm conflicted on whether it's appropriate for gptlint.

Here are two potential solutions:

  1. we remove support for .env. currently it's only used for 1 env var, and you can pass it as export OPENAI_API_KEY or via --apiKey or via the config's llmOptions.apiKey. we could even punt it to users to use dotenv in their gptlint.config.js as opposed to having gptlint do it automatically.
  2. or we keep support for .env and merge this PR

@mergebandit thoughts?

Yeah really good point on not spilling secrets.

I think I prefer the idea of punting to users - giving them maximum control, while de-risking shipping a secret to an LLM. How about I update this PR to remove the usage of dotenv?

transitive-bullshit commented 3 months ago

How about I update this PR to remove the usage of dotenv?

Sounds good 💪

transitive-bullshit commented 3 months ago

Interesting; just learned that node supports .env natively from v20 onwards so we should probably keep .env support.

https://javascript.plainenglish.io/ditch-dotenv-node-js-now-natively-supports-env-file-loading-8b9b2d49b2d2