[options]: Replace --disable-progress-updates with --progress (none | ansi)

[saehejkang]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Part 1 of #641

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

[jglogan]

Let's look at the ansi aspect of this next week.

Right now the progress bar defaults to ansi style behavior (fine grained updates rewrite the same line over and over).

@dkovba would it be best to break this into two PRs?

• The first is this one, with --progress none and --progress ansi values in place of --disable-progress-updates.
• A follow-up PR adds support for plain progress to the TerminalProgress types.

[saehejkang]

Right now the progress bar defaults to ansi style behavior (fine grained updates rewrite the same line over and over).

So to be clear, if we want the progress bar to be disabled, the option would be none? Kind of like how it is set here?

however, the else, after that if statement is setting more options in the ProgressConfig. Would that be the plain or ansi option?

[dkovba]

Thank you for the contribution!

It seems we need to rename plain in the code to ansi.

Closes #641

Without implementing all three options, we shouldn't close the issue.

[dkovba]

• The first is this one, with --progress none and --progress ansi values in place of --disable-progress-updates.
• A follow-up PR adds support for plain progress to the TerminalProgress types.

Two PRs are fine. But the plain option shouldn't take much code to implement and could be done in the single PR. It might help to describe the expected behavior for each option in #641.

[jglogan]

@saehejkang @dkovba I've updated the #641 issue description for just none and ansi, and described other option values for follow-on PRs.

[jglogan]

@saehejkang That looks good. It looks like you'll need to do one more rebase onto main since some changes to the docs got merged today. After that we can get this merged.

[jglogan]

@saehejkang I went ahead and fixed that with the web editor, no need to put you through more of this rebase nonsense 😃

@dkovba Please have a look now as it still requires your ✅

[saehejkang]

@saehejkang I went ahead and fixed that with the web editor, no need to put you through more of this rebase nonsense 😃

I tend to click the merge with main button in PRs, so I am unsure if I should continue doing that? Is it just there should only be 1 merge with main commit (just don't click the button more than once)?

[jglogan]

The commit history should just include your actual feature commit(s); there shouldn't be merge commits from main or any other branch in your PR's branch history.

If your PR branch conflicts with main, just fetch the latest main head from the upstream, merge or rebase it onto the main of your fork, and then rebase your PR branch against that.

[dkovba]

After discussing the guidance for merge commits, we’ve decided that rebasing is not mandatory. We’ll review and accept PRs that include merge commits.

[saehejkang]

I still have a question about how this option below will be updated (looks like the only other place we pass the flag in the BuildCommand).

@Option(name: .long, help: ArgumentHelp("Progress type (format: auto|plain|tty)]", valueName: "type"))
var progress: String = "auto"

Do we want this move to the global flags file? Would it be defined in the Progress struct? If so, would it not have to be named something different than progress? What would we want to name the enum?

[dkovba]

At this moment, the builder is using a separate progress implementation. It can be different from the progress bar in the rest of the project.

[dkovba]

👍