Skip to content
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

Add a progress meter #237

Closed
tarkatronic opened this issue Oct 22, 2021 · 6 comments
Closed

Add a progress meter #237

tarkatronic opened this issue Oct 22, 2021 · 6 comments
Labels
enhancement New feature or request Hacktoberfest

Comments

@tarkatronic
Copy link
Contributor

Feature Request

Is your feature request related to a problem? Please describe.

Right now, when tartufo is running, it can be very silent, and you may not even know that it's still going, or how much progress has been made.

Describe the solution you'd like

It would be nice to be able to specify something like --progress to display a friendly progress meter, showing that things are actually continuing to help, and even showing a rough percentage complete.

Two possible (theoretically) drop-in packages I know of that could fill this feature are:

Describe alternatives you've considered

Live output (#227) helps with this somewhat, in that issues are displayed to the command line immediately, but if you have a very long history with no remaining issues, you may still experience a long runtime with no output.

Teachability, Documentation, Adoption, Migration Strategy

Easy enough, just add a --progress or similar command line switch. I think it should likely default to off. We will definitely need to investigate whether this will play nicely with the live output. I have a nagging suspicion that, if both of these are turned on, they will interfere with each other, since they will have interleaving output.

@tarkatronic tarkatronic added enhancement New feature or request Hacktoberfest labels Oct 22, 2021
@sadielbartholomew
Copy link

Hi there, if nobody is working on this, please could I take this issue on for Hacktoberfest 2021? If so, I would like to use 'tqdm' if there is flexibility over which package to use for ease of incorporating a progress indicator. And, since you state that #227 might interplay, do you know when the PR #227 can be expected to be merged, so I can get a feel for whether I should in that case wait a few days before starting to work on this or not (to do this for Hacktoberfest I need to have the PR up by the end of the week so I would have to start preparing the PR by around Thursday at the latest)?

@tarkatronic
Copy link
Contributor Author

Hi @sadielbartholomew! Sorry I took so long in getting back to you. If you are still interested in working on this, you would be very welcome!! I hope to get #227 merged in today, as we are very actively working on building up our set of features for a v3.0 release.

A couple of things to keep in mind with this:

  • This will need to be branched off from, and merged back into, the v3.x branch
  • We are trying to develop this as a library first, with the CLI just being a wrapper. This means that ideally, the library should not have CLI-specific references. So my hope is that the scanners themselves have some sort of generic progress_max and progress_current property... or, you know, something with better names. And then the CLI would just poll those properties to output the progress bar. I haven't thought too far into how exactly that would work.

That's it!

I'm also curious to hear why tqdm as opposed to click. Either one is welcome though!

@sadielbartholomew
Copy link

Hi @tarkatronic, no worries at all, and sorry I am quite slow to respond again myself (I have left Hacktoberfest very late this year so I am trying to prepare a number of PRs this week on top of my day job, so likely I will get everything finalised at the weekend).

I hope to get #227 merged in today, as we are very actively working on building up our set of features for a v3.0 release.

Great, and I see #227 has now been merged, so that is helpful.

This will need to be branched off from, and merged back into, the v3.x branch

Noted, I've forked the repo and I am using that branch to develop my PR on.

... I haven't thought too far into how exactly that would work.

That all makes sense and I will keep it in mind as I go. Maybe I can open a draft PR with my proposed solution and if it needs major changes I can make those before opening it up for review. I'll see how I get on.

I'm also curious to hear why tqdm as opposed to click. Either one is welcome though!

The only reason is I've played around with tqdm before (though not formally incorporated it into software) so have some familiarity there, and I liked it because the documentation was very clear, but I'll keep an open mind as to which to use.

@rbailey-godaddy
Copy link
Contributor

Hi @sadielbartholomew, I'm in roughly the same place (some tqdm experience, but not with click). I just looked at the click doc and it doesn't seem too dissimilar.

FWIW, I think your biggest obstacle is that .scan() is now a generator with no length, so you will need to calculate a length in advance -- and the counts that are easiest to compute quickly may not correspond directly to any of the inner loop iterators...

@tarkatronic and I had discussed this earlier and felt the best approach (but don't feel limited if you have an idea) is for each scanner class to figure out itself some size estimate (number of commits, number of files, etc.) and then track progress against that with a .progress property that is normalized to 0-100 so the caller doesn't have to make two calls (one for total size and one for current progress). Then in the outer code (i.e. util.echo_result()) where we're iterating on .scan() you can update the progress bar manually.

Regarding intermingling with other output, about the only thought I have at this point is that --progress and --json need to conflict, because obviously progress output will corrupt the "json-ness" of stdout no matter where it appears. ;-)

Thanks!

@sadielbartholomew
Copy link

Thanks for the further info, @rbailey-godaddy! I'll keep it in mind. Expect the PR today or tomorrow.

@rbailey-godaddy
Copy link
Contributor

Fixed by #421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Hacktoberfest
Projects
None yet
Development

No branches or pull requests

3 participants