Skip to content

Conversation

@izvyk
Copy link
Contributor

@izvyk izvyk commented Oct 29, 2025

Description

Closes #191

@izvyk izvyk self-assigned this Oct 29, 2025
@izvyk izvyk linked an issue Oct 29, 2025 that may be closed by this pull request
@izvyk
Copy link
Contributor Author

izvyk commented Nov 17, 2025

In general I feel that this PR is a little more complex than it needs to right now. Are there points to simplify it?

Unfortunately, I don't see any space for meaningful simplification without introducing changes for users.

}

func (c *Context) GetAPIToken() string {
// TODO ensure consistency
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

@vknabel vknabel left a 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.

Comment on lines +828 to +830
// Console tests below

type consoleTestCase[T any] struct {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Gerrit91 Gerrit91 self-requested a review December 1, 2025 13:19
@izvyk izvyk marked this pull request as ready for review January 12, 2026 16:39
@izvyk izvyk requested a review from a team as a code owner January 12, 2026 16:39
@izvyk izvyk merged commit e360b6d into master Jan 12, 2026
2 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Development Jan 12, 2026
@majst01 majst01 deleted the 191-generic-cli-context-handler branch January 13, 2026 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Generic CLI context handler

4 participants