Skip to content

fix: separate title and message in progress bar [IDE-1067] #335

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rrama
Copy link

@rrama rrama commented Apr 22, 2025

Since the language server has to conform to the LSP, we are bound by how the progress bars are defined there, as such it makes sense to align the GAF progress bar with it.

rrama added 4 commits April 22, 2025 12:13
The title must be given at the start of the progress bar and cannot change, whereas the message can change at any time throughout.
This is to be inline with the limitations on progress bars imposed by the LSP.
`SetMessage` and `Clear` usage clarification
@rrama
Copy link
Author

rrama commented Apr 22, 2025

In the future I would like to split the ProgressBar interface into two types of progress bar, one representing infinite progress and other quantifiable progress. Something like:

type UserInterface interface {
	Output(output string) error
	OutputError(err error, opts ...Opts) error
-->     NewProgressBar(title string, message string) ProgressBar
-->     NewProgressBarInfinite(title string, message string) ProgressBarInfinite
	Input(prompt string) (string, error)
}
type ProgressBar interface {
	UpdateProgress(progress float64) error
	UpdateProgressWithMessage(progress float64, message string) error
	Clear() error
}

// ProgressBarInfinite is like ProgressBar, but without a reported percentage,
// so the spinner is infinite.
type ProgressBarInfinite interface {
	SetMessage(message string) error
	Clear() error
}

(It is reasonably trivial to have the underlying consoleProgressBar struct be being duck-typeable to bother interfaces)

But alas, that change ended up being too large for now. So I have parked it here: https://github.com/snyk/go-application-framework/tree/fix/progress-bar-v3

@rrama rrama marked this pull request as ready for review April 22, 2025 14:33
@rrama rrama requested review from a team as code owners April 22, 2025 14:33
@rrama rrama changed the title fix: WIP separate title and message in progress bar [IDE-1067] fix: separate title and message in progress bar [IDE-1067] Apr 22, 2025
// The message is displayed alongside the title passed in by `NewProgressBar(title string)`.
// The message provides context or description for the operation that is being tracked.
// Set to "" to clear the message.
SetMessage(message string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: @rrama did you integrate these changes in the CLI? I assume given that this PR changes interfaces, it'll require changes to existing extensions. Did you consider doing this without a breaking change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSchafer, for the CLI it requires merging the old SetTtitle into the NewProgressBar in cliv2/cmd/cliv2/debug.go and then for cli-extension-iac the same again in internal/commands/iactest/ui.go. I can make PRs ready to merge before this gets merged.

I had considered doing it without any knock on changes to the CLI, it would leave the interface being less intuitive to use, as the LS would have to ignore SetTitles after the first UpdateProgress is called. I am hoping this impact is minor enough, but if not then let me know and I can do a zero knock on changes.

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.

2 participants