-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Extract ASGI scope creation into function #162
Extract ASGI scope creation into function #162
Conversation
Thanks for the PR @emcpow2. There is an issue here related to custom scope resolution that should also be considered when making a change like this, and I think if we decide to go with non-AWS event handling that will take some additional design consideration later as well. I'm going to close this because there is some investigation to do before we start extracting logic from the main adapter method. |
Thit PR does not introduce any complexity regarding the path resolution issue. Instead, the code looks cleaner and easier to test and to introduce new changes. Please consider reopening the PR. |
@emcpow2 I don't disagree with your approach generally, and I've actually thought we should do something like this for awhile now. However, I've held off on these kind of changes because once we introduce new public API then users will start relying on it in ways that conflict with other things we might want to do. This is the same reason I left the linked issue open because the design needs careful consideration. |
@emcpow2 I've taken the time to think about this some more - let's go ahead with it. I'll reopen and do a small review of some adjustments, but otherwise this should be fine to do now. Thanks. |
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 few small things then I think this will be good to merge. The most significant change request is to remove the separate method for handling the initial body (we can look at this as a separate issue later).
b0c8bc4
to
16371fa
Compare
extract scope and request body creation into a dedicated functions
a5b55de
to
191b478
Compare
191b478
to
5c77151
Compare
thanks. Done |
Looks good, thanks! |
* 🏗 simplify magnum call function extract scope and request body creation into a dedicated functions * 📝 add documentation to create scope function
* 🏗 simplify magnum call function extract scope and request body creation into a dedicated functions * 📝 add documentation to create scope function
* 🏗 simplify magnum call function extract scope and request body creation into a dedicated functions * 📝 add documentation to create scope function
This is a required abstraction to create non-AWS event handlers.