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

stricter lints #1597

Merged
merged 6 commits into from
Nov 17, 2023
Merged

stricter lints #1597

merged 6 commits into from
Nov 17, 2023

Conversation

nico-famedly
Copy link
Member

@nico-famedly nico-famedly commented Nov 2, 2023

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?

@nico-famedly nico-famedly force-pushed the nico/stricter-lints branch 2 times, most recently from f9cfa17 to 1e57d21 Compare November 3, 2023 14:18
@nico-famedly nico-famedly marked this pull request as ready for review November 3, 2023 14:26
Copy link
Contributor

@techno-disaster techno-disaster left a 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

@nico-famedly
Copy link
Member Author

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.

analysis_options.yaml Outdated Show resolved Hide resolved
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.
krille-chan
krille-chan previously approved these changes Nov 17, 2023
@nico-famedly nico-famedly merged commit ee14fb7 into main Nov 17, 2023
9 checks passed
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

Successfully merging this pull request may close these issues.

3 participants