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

elide some asyncs #1665

Closed
wants to merge 1 commit into from

Conversation

SimonCropp
Copy link
Contributor

I look through the existing codebase and it seems the preference is to elide asyncs where possible. to here are some that have been missed

@josephdecock
Copy link
Member

We don't have a clear decision on style for this. Your other PR where you pointed out that sometimes eliding async affects timing of activities makes me think we shouldn't change this without really carefully reviewing each change to make sure we're not doing something like that. But since we don't even have a consensus on which way we would go, I'm just going to close this - thanks anyway!

@SimonCropp SimonCropp deleted the elide-some-asyncs branch December 10, 2024 04:02
@SimonCropp
Copy link
Contributor Author

shouldn't change this without really carefully reviewing each change to make sure we're not doing something like that

i was very careful. i reviewed each case to ensure it would not have those side effects

@josephdecock
Copy link
Member

Oh I'm sure you were careful, I didn't mean to imply otherwise. I was just noting that the code review would need to be quite careful as well - but since we don't have a commitment to this particular style, I don't think we need to make this change right now.

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.

2 participants