-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Conversation
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
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:
(It is reasonably trivial to have the underlying 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 |
// 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) |
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.
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?
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.
@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 SetTitle
s 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.
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.