-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-3120 Replace toPromise with lastValueFrom #2918
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2918 +/- ##
==========================================
- Coverage 80.90% 80.90% -0.01%
==========================================
Files 533 533
Lines 31201 31200 -1
Branches 5082 5077 -5
==========================================
- Hits 25244 25243 -1
Misses 5196 5196
Partials 761 761 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits!
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/retrying-request.service.ts
line 99 at r1 (raw file):
private async invoke(options: FetchOptions): Promise<T | undefined> { while (!this.canceled && this.status !== 'complete') { const online = await lastValueFrom(this.online$.pipe(take(1)));
NIT: This could be simplified as:
const online = await firstValueFrom(this.online$);
Code quote:
const online = await lastValueFrom(this.online$.pipe(take(1)));
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/retrying-request.service.ts
line 132 at r1 (raw file):
take(1) ) );
NIT: This could also be simplified as:
await firstValueFrom(this.online$.pipe(filter(isOnline => isOnline)));
Code quote:
await lastValueFrom(
this.online$.pipe(
filter(isOnline => isOnline),
take(1)
)
);
5b1f10f
to
b6b79be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two make sense, thanks
Reviewable status: 9 of 11 files reviewed, all discussions resolved (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/retrying-request.service.ts
line 99 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: This could be simplified as:
const online = await firstValueFrom(this.online$);
Done, thanks
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/retrying-request.service.ts
line 132 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: This could also be simplified as:
await firstValueFrom(this.online$.pipe(filter(isOnline => isOnline)));
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
93f58f1
to
f9d5bdd
Compare
This change is