-
-
Notifications
You must be signed in to change notification settings - Fork 563
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: Allow multiple telegram IDs #52
Conversation
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.
LGTM! Since you're adding tests, could you add an Actions workflow that runs them? 😄
Also, it is not super clear to me if this still allows leaving the ID field empty & specifying a single ID, could we get tests for those as well? :D |
Hey @m1guelpf, yeah good shout on both, moved to table tests as there are a few now. Action added 😉 Left the test action to run on |
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.
LGTM! Just some final comments before we get this in 😄
src/config/env_config.go
Outdated
if !fileExists(path) { | ||
return nil, fmt.Errorf("config file '%s' does not exist", path) | ||
} |
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.
Following #46, we want to proceed when the .env
file isn't found (but print a warning just in case)
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.
there is a slight issue with this (a quirk of viper). it requires reading a config into viper to register the keys, only these keys will then be loaded from env.
imo we should rename env.example
to .env
and keep the failure.
One other alternative would be to register the keys by encoding an empty instance of the EnvConfig
we have, before reading from env, wdyt @m1guelpf ?
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.
@m1guelpf updated with a somewhat of a middle ground, introduced an emptyConfig
string for the purpose of registering the keys.
Test added for when no file is provided
want: &EnvConfig{ | ||
TelegramID: []int64{}, | ||
TelegramToken: "", | ||
EditWaitSeconds: 0, |
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.
This should default to 1 right?
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.
defaults to 1 in the Validate
method
i received the following error running on Mac Mini M1. i am unsure how to troubleshoot it or what went wrong. i have edited the env.example to env.env according to the instructions. |
copy |
Moved to loading all of env file variables with viper. This provides the nice benefit of type support.
Updated existing
config.Init
to have a more explicit name and use an instance of viper rather than the global.#41