-
Notifications
You must be signed in to change notification settings - Fork 39
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
stricter lints #1597
stricter lints #1597
Conversation
f9cfa17
to
1e57d21
Compare
2f59072
to
55a52ab
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.
Mostly LGTM, can you explain why some of the functions don't run in root anymore? updateSessionUsage
for example
I don't see why they should be running in root, when you catch all their exceptions. I always thought runInRoot was for background requests,but those cases clearly were not supposed to run in the background. So I refactored runInRoot to not return a future and run stuff as a background request, which of course made it required to move stuff out, where we want to await the response. |
55a52ab
to
1506395
Compare
BREAKING CHANGE: This changes the runInRoot method to not return a future. As a user, if you need the result of an async computation passed to runInRoot, please await it directly. Also the KeyVerification start and a few call methods now return a future.
1506395
to
f72d298
Compare
I guess the only controversial change is the runInRoot stuff, but I don't see, why we would ever want to await those. The few cases, where we do, seem to fit better when we just await them directly?