-
Notifications
You must be signed in to change notification settings - Fork 1
Implement shared context command in genericCLI #194
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
base: master
Are you sure you want to change the base?
Conversation
…e connect & Msg
…ake of consistency
Unfortunately, I don't see any space for meaningful simplification without introducing changes for users. |
| } | ||
|
|
||
| func (c *Context) GetAPIToken() string { | ||
| // TODO ensure consistency |
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.
Can you elaborate on this TODO?
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.
CLI tends to fallback to cmd arguments, but this patters is not universal. Are there any requirements/ideas on this behaviour?
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.
Not sure where the consistency issues is here.
In general we have the following priorities:
- if there is a flag, use it (viper takes care of this)
- if there is an env, use it (viper takes care of this)
- rely on the config file
vknabel
left a comment
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.
Test structure looks good IMHO. Will still have a more in depth look on the individual test cases.
| // Console tests below | ||
|
|
||
| type consoleTestCase[T any] struct { |
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.
Maybe, and I am not sure, it makes sense to create two separate test files here. One for the context manager tests from the usage perspective and one from the console perspective.
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 had the same idea initially, but I decided to keep it in one file for better integration. The IDE automatically picks it up as the test file for context.go.
…m implementation for the sake of consistency
Description
Closes #191