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

add executor to validate_payload #652

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Conversation

drc38
Copy link
Contributor

@drc38 drc38 commented Jun 24, 2024

Addresses issue #647

@jainmohit2001
Copy link
Collaborator

Hi @drc38, the changes look good. I believe the functionality will not break and it is backwards compatible as well.

Here's another thought: Instead of using run_in_executor to run the blocking validate_payload function call, what if we convert the validate_payload function into a pure async function with all I/O calls being non-blocking.

@drc38
Copy link
Contributor Author

drc38 commented Sep 24, 2024

Hi @drc38, the changes look good. I believe the functionality will not break and it is backwards compatible as well.

Here's another thought: Instead of using run_in_executor to run the blocking validate_payload function call, what if we convert the validate_payload function into a pure async function with all I/O calls being non-blocking.

That would result in quite a departure from the current structure where most functions are not async. Another approach is to use https://github.com/Tinche/aiofiles for the file open, assuming you're happy to import aiofiles as part of the project.

@drc38
Copy link
Contributor Author

drc38 commented Sep 26, 2024

That would result in quite a departure from the current structure where most functions are not async. Another approach is to use https://github.com/Tinche/aiofiles for the file open, assuming you're happy to import aiofiles as part of the project.

Actually using aiofiles would still require a rewrite to async. I think the proposed approach is the best for now.

@jainmohit2001 jainmohit2001 added the enhancement New feature or request label Oct 1, 2024
@jainmohit2001 jainmohit2001 requested review from proelke and removed request for OrangeTux and Jared-Newell-Mobility October 15, 2024 10:49
@jainmohit2001
Copy link
Collaborator

jainmohit2001 commented Oct 15, 2024

LGTM.
@proelke let's merge this PR.

@proelke proelke merged commit 71c5adb into mobilityhouse:master Oct 15, 2024
6 checks passed
@drc38 drc38 deleted the block-fix branch October 15, 2024 17:59
@drc38
Copy link
Contributor Author

drc38 commented Oct 17, 2024

@proelke @jainmohit2001 Thank you both for continuing to maintain the library, are we able to get a PyPi release that includes this merge please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants