-
-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for loading dotenv files #73
Conversation
if (process.env.TOLGEE_API_KEY) { | ||
setOptionValue('apiKey', process.env.TOLGEE_API_KEY); | ||
} | ||
if (process.env.TOLGEE_API_URL) { | ||
setOptionValue('apiUrl', parseUrlArgument(process.env.TOLGEE_API_URL)); | ||
} | ||
if (process.env.TOLGEE_PROJECT_ID) { | ||
setOptionValue( | ||
'projectId', | ||
parseProjectId(process.env.TOLGEE_PROJECT_ID) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure how to feel about that. I'm aware we can't fetch env variables be fetched before because we need to get --env
, however doing manually the env checks does duplicate the places where it needs to be handled and is prone to desync...
Iterating over the list of available options stored in program
is probably better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do a followup refactoring PR to handle loading the configuration in one place, potentially with a schema library, like zod, which would further simplify things.
The reason I did not iterate here, because different options need different parsing function, like the TOLGEE_PROJECT_ID
needs the parseProjectId()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the parse function already stored and accessible via the list of available options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to show me a code snippet on how we could improve this function? I might just be slow today, but it would help me for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to get the list of options that can be specified on a given command using the options
field; considering this I wonder if it's not possible to use this (along with parent
since commands are structured in a tree-like structure) to get all the options that can be specified, and check if the option can use an env variable, if so use its parser and its name()
to populate the command options - see this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't find a way to iterate over the options and set the env variables that way. It is not possible to get whether an option can use an env variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, sorry for the late reply, I unfortunately got sick 🤧
I was able to iterate over all the options using this snippet:
function processCommand (command: Command) {
for (const option of command.options) {
console.log(option.attributeName(), option.envVar)
}
if (command.parent) {
processCommand(command.parent)
}
}
Closes #56
This PR adds a new option to the CLI,
--env
, which specifies which env file to load before a command is executed.