Skip to content

Conversation

@mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Mar 6, 2022

Allow Line.eta to display a duration for slow processes.

This experiments with @pveber proposal from #23, namely changing Flow_meter.per_second to return a float.

Note, the eta does not refresh as time passes, unlike Line.elapse, so possibly more changes are needed to improve further the display for slow processes (not done here).

I tested it with a slow process on my machine, and witnessed the display go from --:-- to an actual value after 2 reported updates.

@dinosaure
Copy link
Collaborator

It seems indeed a relevant pull-request. I will merge it and add it into the next release.

@mbarbin
Copy link
Contributor Author

mbarbin commented Apr 16, 2024

Hi @dinosaure Thanks for the recent release of 0.3.0 🚀 . Would you mind telling a few words regarding this PR, and the reason it wasn't included. What's holding it? I'm happy to have a fresh look. Thanks!

@dinosaure
Copy link
Collaborator

Hi @dinosaure Thanks for the recent release of 0.3.0 🚀 . Would you mind telling a few words regarding this PR, and the reason it wasn't included. What's holding it? I'm happy to have a fresh look. Thanks!

Hi @mbarbin, first, thanks for your work. I took a long time because I need to convince myself about the usage of a float instead of an Integer.t. As I said, I'm not particularly focused on performance and the use described by @pveber seems legitimate to me. I'm going to break the API (again), so it seems appropriate to integrate this PR from now on.

@dinosaure dinosaure merged commit 1f4e666 into craigfe:main May 20, 2024
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request May 20, 2024
CHANGES:

- Revert the `terminal` API and keep an "happy" path to get size of a tty
  and be compatible with MirageOS (@art-w, @msprotz, craigfe/progress#42, craigfe/progress#43)
- Use a `float` instead of a `int` in `flow_meter per-second` (@mbarbin, craigfe/progress#23, craigfe/progress#27)
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