Skip to content

feat: add support for loading dotenv files#73

Closed
Lehoczky wants to merge 5 commits intotolgee:mainfrom
Lehoczky:main
Closed

feat: add support for loading dotenv files#73
Lehoczky wants to merge 5 commits intotolgee:mainfrom
Lehoczky:main

Conversation

@Lehoczky
Copy link
Copy Markdown
Contributor

@Lehoczky Lehoczky commented Feb 26, 2024

Closes #56

This PR adds a new option to the CLI, --env, which specifies which env file to load before a command is executed.

@Lehoczky Lehoczky marked this pull request as draft February 26, 2024 19:23
@Lehoczky Lehoczky marked this pull request as ready for review February 26, 2024 19:55
Comment thread src/program.ts
Comment on lines +142 to +153
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)
);
}
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@Lehoczky Lehoczky Feb 28, 2024

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().

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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)
  }
}

@Lehoczky Lehoczky closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native support for .env files

2 participants