👷(CLDSRV-860) Monitor async await migration#6088
👷(CLDSRV-860) Monitor async await migration#6088DarkIsDude wants to merge 8 commits intodevelopment/9.3from
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DarkIsDude
force-pushed
the
feature/CLDSRV-860/monitor-async-await-migration
branch
from
93020af to
8981239
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
.github/scripts/check-diff-async.mjs
Outdated
| @@ -0,0 +1,142 @@ | |||
| /** | |||
| * Check that all new/modified functions in the current git diff use async/await. | |||
| * Fails with exit code 1 if any additions introduce callback-style functions or .then() chains. | |||
There was a problem hiding this comment.
can't we use https://github.com/tj-actions/eslint-changed-files for this purpose?
seems quite similar...
or run eslint like this: eslint --fix $(git diff --name-only HEAD | xargs) (c.f. https://stackoverflow.com/questions/54511168/enable-eslint-only-for-edited-files)
There was a problem hiding this comment.
I don't want to run it only on the file but also on the line. We have pretty big file with big function and changing all of them can be a mess 😬. What do you think @francoisferrand ?
| } | ||
| } | ||
|
|
||
| const migrationPercent = totalFunctions > 0 |
There was a problem hiding this comment.
this is not correct : not all functions will be (ever) be async.
the migration progress is really 1 - callbackFunctions / (callbackFunctions_before_migration)....
as a "kind of proxy", we may compute callbackFunctions / (asyncFunctions + callbackFunctions) ; but not sure it is that useful...
There was a problem hiding this comment.
(completion is really just when callbackFunctions == 0)
There was a problem hiding this comment.
Changed. The goal here is just to have an indicator about progress (maybe to be able to follow that over time) not to have something really accurate. Reaching 100% is not the goal, the real goal is the 0... I would like to keep the calcul simple
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DarkIsDude
force-pushed
the
feature/CLDSRV-860/monitor-async-await-migration
branch
from
a64c535 to
f1e7018
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DarkIsDude
force-pushed
the
feature/CLDSRV-860/monitor-async-await-migration
branch
from
f1e7018 to
c4d81e1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DarkIsDude
force-pushed
the
feature/CLDSRV-860/monitor-async-await-migration
branch
from
f3ff7ea to
8612e12
Compare
This comment was marked as resolved.
This comment was marked as resolved.
3 participants
Pull request template
Description
Motivation and context
Why is this change required? What problem does it solve?
Related issues
Please use the following link syntaxes #600 to reference issues in the
current repository
Checklist
Add tests to cover the changes
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Sign your work
In order to contribute to the project, you must sign your work
https://github.com/scality/Guidelines/blob/master/CONTRIBUTING.md#sign-your-work
Thank you again for contributing! We will try to test and integrate the change
as soon as we can.