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

Added text_mime_types argument #277

Merged
merged 7 commits into from
Oct 16, 2022
Merged

Added text_mime_types argument #277

merged 7 commits into from
Oct 16, 2022

Conversation

georgebv
Copy link
Contributor

@georgebv georgebv commented Oct 1, 2022

Added optional text_mime_types argument to:

  • Mangum adapter's __init__ method
  • handle_base64_response_body function (this function is the target recipient of the text_mime_types list)

This argument is optional and by default references the same DEFAULT_TEXT_MIME_TYPES list.

WARNING: old solutions involving editing the DEFAULT_TEXT_MIME_TYPES list are now broken because this list was moved from mangum/handlers/utils.py to mangum/adapter.py.

resolves #275

Copy link
Owner

@jordaneremieff jordaneremieff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @georgebv! Just a few requests related to how the configuration and default are organized now.

The original intention of the LambdaConfig was for things like this to avoid having to pass things explicitly to the handler __call__ methods.

Also, could you add one new test demonstrating the new configuration being used?

mangum/adapter.py Outdated Show resolved Hide resolved
mangum/handlers/utils.py Outdated Show resolved Hide resolved
mangum/adapter.py Outdated Show resolved Hide resolved
@georgebv
Copy link
Contributor Author

georgebv commented Oct 2, 2022

Regarding new tests, I wasn't sure where to put this. Technically, this is an integration test where we use adapter itself. But, since handlers are so different, I put the test to API gateway.

mangum/adapter.py Outdated Show resolved Hide resolved
@jordaneremieff
Copy link
Owner

Regarding new tests, I wasn't sure where to put this. Technically, this is an integration test where we use adapter itself. But, since handlers are so different, I put the test to API gateway.

Yep that’s fine, thank you.

@jordaneremieff
Copy link
Owner

Hi @georgebv, are you still interested in working on this feature?

@georgebv
Copy link
Contributor Author

@jordaneremieff absolutely, didn't have time these last 2 weeks - was planning to wrap this up in the next few days

@georgebv
Copy link
Contributor Author

I had to bring back the [*DEFAULT_TEXT_MIME_TYPES] because tests testing it were failing due to global list being modified by handler.config["text_mime_types"].append(content_type.decode()). Let me know if you have a better solution for this, but I think this is the best way to handle this - we use global list as default and we make a copy of it on adapter instantiation. Without this the __init__ method of Mangum would have this side effect of mutating global object.

@jordaneremieff
Copy link
Owner

I had to bring back the [*DEFAULT_TEXT_MIME_TYPES] because tests testing it were failing due to global list being modified by handler.config["text_mime_types"].append(content_type.decode()).

Yep, that's fine then, thanks for explaining.

Copy link
Owner

@jordaneremieff jordaneremieff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great work!

@jordaneremieff jordaneremieff merged commit f872706 into jordaneremieff:main Oct 16, 2022
khamaileon pushed a commit to khamaileon/mangum that referenced this pull request Jan 13, 2024
* added vscode files to gitignore

* added text_mime_types argument

* updated text mime type definition and propagation

* copy default text mime types inside mangum

* api gateway test

* moved instance attributes to config

* added tests for custom text mime types
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.

Allow providing extra DEFAULT_TEXT_MIME_TYPES
2 participants