serokell/xrefcheck

Refactor the interface to progress bar updates

Martoon-00 opened this issue · 3 comments

Martoon-00 commented

Clarification and motivation

For providing a progress bar interface we provide functions: incProgress, incProgressUnfixableErrors, decProgressFixableErrors, but they constitute quite a low-level interface. If I use incProgressFixableErrors, it should be accompanied by incProgress, forgetting it is likely an error.

fixableToUnfixable is better in this regard.

Let's discuss in comments how to better improve this situation.

Acceptance criteria

  • We provide a convenient high-level interface to changing progress bar state that is not error-prone to a reasonable extent.
  • All the related code (progress bar updates and maybe even uses of Progress constructor to make a datatype) is updated to use this new interface.
Martoon-00 commented
My proposal

I think what humans expect here is (just illustrating idea, this might eventually look differently):

data ProgressItemStatus = Unchecked | Ok | Fixable | Failed

(->) :: ProgressItemStatus -> ProgressItemStatus -> Progress a -> Progress a
-- example 
let progress' = progress & Unchecked -> Ok

This way, intention behind some of the updates may be expressed more clearly:

progressUpdateOnRetry =
  (if retryCounter == 0 then Unchecked else Fixable)
  ->
  (if retryCounter < maxRetries then Fixable else Failed)
Martoon-00 commented
Another, more complex but better proposal

With both the current solution and the improvement I suggested above there is an issue - we have to figure out the old status correctly for things to work. And this is pretty error-prone, not saying that it may complicate future features (e.g. if we decide to track redirects too).

So instead we could internally keep Map Link ProgressStatus and each link will overwrite its own status.
Maybe instead of using Link as key (I'm not sure how we currently reflect duplicated links in progress bar) there should be a unique token instead.

  • 👍1
aeqz commented

Regarding the second proposal, it looks really natural, but creating unique tokens for each verification item can require great changes, because I guess that these would be created when initialising the progress bar and then each verification item should keep track of its own token.

I'm not sure how we currently reflect duplicated links in progress bar

Currently, we count unique links to calculate the total amount of work to do, and then the verification process implements a request/response cache for external resources, so it seems that duplicated verification of them is avoided.

I have proposed another solution in #265, inspired by that one, which I think that is simple, does not require great changes and also makes the progress-bar interface less low-level and error prone.

  • 👀1