search

bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
3.86k stars 224 forks source link

Show days in ETA if needed #1043

Open mvglasow opened 1 month ago

mvglasow commented 1 month ago

As discussed in #1038. This PR adds the following functionality:

If ETA >= 86220 s (which rounds up to 86400 s, or 24 h, for display purposes), it is displayed as:

0%  0/1  (40.3 MiB of 263.59 GiB)   357 KiB/s    2d 20:25:00 ETA

ETAs below that threshold will get displayed as before.

Since this is my first time messing with any OCaml code, you might want to look at the syntax. Lines 496–501 are truly new, the others just had indentation added. In particular, I am not sure if the parentheses around the tuple members in 497/498 are correct, or if they’re needed at all.

I have run a brief test with ETAs both below and above the threshold, and it builds and works as intended.

tleedjarv commented 1 month ago

@mvglasow maybe that's just how GitHub's suggestions work but you don't have to assign any co-authorship to me and can simply squash the commits.

gdt commented 1 month ago

This PR is about changing large numbers of hours into days and hours. I am fine with that. I do not want to have a change to display rounded times in the same PR; that's a different discussion that we don't even have an issue about :-)

gdt commented 3 weeks ago

Agreed that varying between hh:mm and mm:ss is not ok.

I feel that the rounding we already have is overly aggressive. Agreed that fixing that is separate. But I'd like to not pile on and make it worse. If we already have "round to 5 minutes" for times that are most of a day, flipping to 30 minutes at a day seems too much (4%). (Yes, it might be that the existing breakpoint for 5m is also too much, but we're not changing that.) If this just doesn't change rounding from how it is (leaving rounding at 5 for multiday), I'm fine with it.

mvglasow commented 3 weeks ago

I’m fine with keeping precision at 5 minutes for ETAs beyond 24 h for now.

gdt commented 3 weeks ago

Great then please adjust, rebase, and make the commit message give rationale for the change. I don't mean anything super extensive, but it's a wee bit too brief as is

mvglasow commented 3 weeks ago

Just committed. Github should allow a squash merge, which will squash all the commits from my branch into a single one – should be the easiest way. If not, give me a heads-up and I’ll see if I can rewrite my branch history.

gdt commented 3 weeks ago

I do not expect github squash to do a good job with a merged commit message turning into the right message for posterity, and we do tend to have merge commits, so I think you can just do a rebase -i, p/s/s and force-push.

mvglasow commented 3 weeks ago

OK. git rebase didn’t do anything as master has not changed, so I did git reset master, then re-committed the differences and force-pushed them to my branch. Commit message is the same as the subject of this PR, which should accurately describe what we’re doing (and, afaik, is also what Github uses as the commit message for a squash commit).

gdt commented 3 weeks ago

The commit message is only really understandable once you have the context. Let me think about how to say enough to orient people who have no idea. It's allowed to have more lines after the 54-byte summary :-) I'll wait a day or so for anybody else to comment.

(I meant git rebase -i origin/master which will give you a chance to specify how you want things in terms of squashing, fixups and reordering, vs rebase which will not.) Thanks for pointing out about github and squash, but I basically think it's a bug we are github and don't want to rely on such behavior anyway, trying to be closer to pure git when reasonable.