Summary: | [webkitcorepy] Retry downloads in Autoinstall | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aakash_jain, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jonathan Bedard
2021-06-17 11:30:42 PDT
Created attachment 431693 [details]
Patch
Comment on attachment 431693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431693&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:100 > + while count <= (AutoInstall.retry or 0): Do we need 0 here, can we just ensure AutoInstall.retry is always valid? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:114 > + if count > (AutoInstall.retry or 0): Ditto > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:393 > + retry = 1 might be better to name it something like: number_of_retry > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:533 > + def set_retry(cls, retry): This method seems unused Comment on attachment 431693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431693&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:100 >> + while count <= (AutoInstall.retry or 0): > > Do we need 0 here, can we just ensure AutoInstall.retry is always valid? This is to make sure we behave correctly in case retry is "None", and given that the variable doesn't have an _, None feels like a reasonable value >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:393 >> + retry = 1 > > might be better to name it something like: number_of_retry Will change it's name >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:533 >> + def set_retry(cls, retry): > > This method seems unused It is at the moment, but given this is part of a library, it seems like something someone might one to change at some point in the future Comment on attachment 431693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431693&action=review >>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:100 >>> + while count <= (AutoInstall.retry or 0): >> >> Do we need 0 here, can we just ensure AutoInstall.retry is always valid? > > This is to make sure we behave correctly in case retry is "None", and given that the variable doesn't have an _, None feels like a reasonable value we can change the default value of retry to 0 (In reply to Aakash Jain from comment #5) > Comment on attachment 431693 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431693&action=review > > >>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:100 > >>> + while count <= (AutoInstall.retry or 0): > >> > >> Do we need 0 here, can we just ensure AutoInstall.retry is always valid? > > > > This is to make sure we behave correctly in case retry is "None", and given that the variable doesn't have an _, None feels like a reasonable value > > we can change the default value of retry to 0 I think we want the default value to be 1. The problem here is if a caller of the library sets "retry" (or "times_to_retry" as will be in the next patch) to "None". Actually, the solution here might be to drop the `set_retry` function, but keep doing something sane when we get a value of "None". Created attachment 431718 [details]
Patch
Committed r279009 (238934@main): <https://commits.webkit.org/238934@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431718 [details]. |