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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Generic CLI context handler

4 participants