Skip to content

progress: remove *progress.Meter#2762

Merged
ttaylorr merged 12 commits intomasterfrom
progress-remove-meter
Dec 8, 2017
Merged

progress: remove *progress.Meter#2762
ttaylorr merged 12 commits intomasterfrom
progress-remove-meter

Conversation

@ttaylorr
Copy link
Contributor

@ttaylorr ttaylorr commented Nov 29, 2017

This pull request does the scaffolding necessary to, and removes the *progress.Meter implementation from package progress.

My initial approach with this branch was to copy verbatim the *progress.ProgressMeter type from progress to package tq. This worked fine, but I noticed that it expanded package tq's API significantly, with the majority of API changes coming from progress.DryRun, progress.WithOSEnv, (etc) options. To mitigate this, my first inclination was to create a sub-package tq/progress, which would house these types/functions. Wanting not to create a new package, I removed the progress meter option functions in favor of an options struct, and moved that into the tq package directly.

Here's a quick breakdown of what happened:

  1. f411d01: merge in the changes from progress-remove-logger to avoid a merge conflict later when merging this branch into master.
  2. 145d0de, 622c7d3: teach *ProgressMeter to respond to the nil-receiver, to make it possible to remove progress.Noop(), and instead use (nil)(*progress.Meter).
  3. a549105: remove progress.Noop().
  4. fa1d512: since *ProgressMeter is the only remaining implementation of progress.Meter, remove the progress.Meter interface.
  5. e35d20f: rename *progress.ProgressMeter -> *progress.Meter to avoid stuttering.
  6. 6521683: to reduce the scope of the *progress.Meter-related API, remove option functions in favor of a struct-style instantiation, i,e.,
m := progress.NewMeter(&progress.MeterOption{
        DryRun: false,
        // ...
})
  1. 54fb383: finally, move *progress.Meter to *tq.Meter, thus enabling the progress package to be removed.

/cc @git-lfs/core

@ttaylorr ttaylorr added this to the v2.4.0 milestone Nov 29, 2017
technoweenie
technoweenie reviewed Dec 5, 2017
View reviewed changes
tq/meter.go Outdated
}

// NewMeter creates a new Meter.
func NewMeter(opt *MeterOption) *Meter {
Copy link
Contributor

@technoweenie technoweenie Dec 5, 2017

Choose a reason for hiding this comment

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

Do we really need the MeterOption type? The only two things to change are whether dry run is enabled, and what logger is used.

m := NewMeter()
m.DryRun = true
m.Logger = tq.LoggerToFile(filename)
m.Logger = tq.LoggerFromEnv(osEnv)
m.Start()

My hunch is that we didn't use fields like this to prevent race conditions. Should we continue trying to protect the caller, or can we simplify things and just document that those fields should be set before Start() is called? FWIW I do prefer the MeterOptions type to the option func args 👍

tq/meter.go Outdated

// Start begins sending status updates to the optional log file, and stdout.
func (p *ProgressMeter) Start() {
func (p *Meter) Start() {
Copy link
Contributor

@technoweenie technoweenie Dec 5, 2017

Choose a reason for hiding this comment

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

Super nitpick: Can you change this to m *Meter?

@technoweenie
Copy link
Contributor

technoweenie commented Dec 7, 2017

I made the suggested fixes so I could see if it worked in #2774. If you really want to keep MeterOptions, we can force push it away.

@ttaylorr
Copy link
Contributor Author

ttaylorr commented Dec 8, 2017

Those changes look great, thanks!

@ttaylorr ttaylorr merged commit b701aa8 into master Dec 8, 2017
@ttaylorr ttaylorr deleted the progress-remove-meter branch December 8, 2017 17:42
@technoweenie technoweenie mentioned this pull request Jan 4, 2018
Merged
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,t: drop prune progress prefix
aed93d8 
In PR git-lfs#2757 the output of the "git lfs prune" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs prune" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "prune: 8 local objects, 9 retained, done."

Two other Git LFS commands follow the same design, the "git lfs fetch"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "prune:" prefix from the messages output by the
"git lfs prune" command.  We expect to revise the progress messages
from the "git lfs fetch" and "git lfs migrate" commands in the same
way in subsequent commits of this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,docs: drop fetch progress prefix
1c0e89a 
In PR git-lfs#2758 the output of the "git lfs fetch" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs fetch" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "fetch: Fetching reference refs/heads/main".

Two other Git LFS commands follow the same design, the "git lfs prune"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "fetch:" prefix from the messages output by the
"git lfs fetch" command.  We have similarly altered the progress messages
of the "git lfs prune" command in a previous commit in this PR, and we
expect to revise the progress messages from the "git lfs migrate"
command in the same way in a subsequent commit in this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,t: drop prune progress prefix
8c13ecb 
In PR git-lfs#2757 the output of the "git lfs prune" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs prune" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "prune: 8 local objects, 9 retained, done."

Two other Git LFS commands follow the same design, the "git lfs fetch"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "prune:" prefix from the messages output by the
"git lfs prune" command.  We expect to revise the progress messages
from the "git lfs fetch" and "git lfs migrate" commands in the same
way in subsequent commits of this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,docs: drop fetch progress prefix
8488488 
In PR git-lfs#2758 the output of the "git lfs fetch" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs fetch" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "fetch: Fetching reference refs/heads/main".

Two other Git LFS commands follow the same design, the "git lfs prune"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "fetch:" prefix from the messages output by the
"git lfs fetch" command.  We have similarly altered the progress messages
of the "git lfs prune" command in a previous commit in this PR, and we
expect to revise the progress messages from the "git lfs migrate"
command in the same way in a subsequent commit in this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,t: drop prune progress prefix
a7f4d0d 
In PR git-lfs#2757 the output of the "git lfs prune" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs prune" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "prune: 8 local objects, 9 retained, done."

Two other Git LFS commands follow the same design, the "git lfs fetch"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "prune:" prefix from the messages output by the
"git lfs prune" command.  We expect to revise the progress messages
from the "git lfs fetch" and "git lfs migrate" commands in the same
way in subsequent commits of this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Feb 25, 2025
@chrisd8088
commands,docs: drop fetch progress prefix
4fde17e 
In PR git-lfs#2758 the output of the "git lfs fetch" command was revised to
use the methods of the Logger structure of the "tasklog" package to
output progress messages, replacing use of the Spinner structure from
the older "progress" package.  The Spinner structure and its methods
were then removed in PR git-lfs#2759, and the "progress" package was removed
entirely in PR git-lfs#2762.

As part of this change, the "git lfs fetch" command's progress messages
were reformatted so as to always begin with the Git LFS subcommand name,
for example, "fetch: Fetching reference refs/heads/main".

Two other Git LFS commands follow the same design, the "git lfs prune"
and "git lfs migrate" commands, but no others.  Git commands also do
not prefix their progress messages with the name of the subcommand
(although messages from a Git remote may be reported with the prefix
"remote:").

To help bring all our commands' progress messages into alignment, we
simply remove the "fetch:" prefix from the messages output by the
"git lfs fetch" command.  We have similarly altered the progress messages
of the "git lfs prune" command in a previous commit in this PR, and we
expect to revise the progress messages from the "git lfs migrate"
command in the same way in a subsequent commit in this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@technoweenie technoweenie technoweenie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

review

Projects

None yet

Milestone

v2.4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ttaylorr @technoweenie