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

Refactor the interface to progress bar updates #235

Open
Martoon-00 opened this issue Dec 10, 2022 · 3 comments
Open

Refactor the interface to progress bar updates #235

Martoon-00 opened this issue Dec 10, 2022 · 3 comments

Comments

@Martoon-00
Copy link
Member

Martoon-00 commented Dec 10, 2022

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
Copy link
Member Author

Martoon-00 commented Dec 10, 2022

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
Copy link
Member Author

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.

aeqz added a commit that referenced this issue Jan 12, 2023
Problem: the current progress bar interface is quite low-level and
error-prone.

Solution: the proposed solution here modifies both the progress bar
internal model and interface by representing process units by witnesses.
It does not require great changes, but the resulting interface is more
clear and less error-prone.
aeqz added a commit that referenced this issue Jan 12, 2023
Problem: the current progress bar interface is quite low-level and
error-prone.

Solution: the proposed solution here modifies both the progress bar
internal model and interface by representing progress units by witnesses
It does not require great changes, but the resulting interface is more
clear and less error-prone.
@aeqz
Copy link
Contributor

aeqz commented Jan 12, 2023

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.

aeqz added a commit that referenced this issue Jan 18, 2023
Problem: The current progress bar interface is quite low-level and
error-prone.

Solution: The proposed solution here modifies both the progress bar
internal model and interface by representing progress units by witnesses
It does not require great changes, but the resulting interface is more
clear and less error-prone.
aeqz added a commit that referenced this issue Jan 20, 2023
Problem: The current progress bar interface is quite low-level and
error-prone.

Solution: The proposed solution here modifies both the progress bar
internal model and interface by representing progress units by
witnesses. It does not require great changes, but the resulting
interface is more clear and less error-prone.
aeqz added a commit that referenced this issue Jan 20, 2023
…-interface

[#235] Refactor progress bar interface
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

No branches or pull requests

2 participants