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

Draft: introduce strict mode #88

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

hbusul
Copy link
Contributor

@hbusul hbusul commented Sep 11, 2023

This is for issue #87
I've added a first draft of the strict mode, to sum up the changes:

In parser.py I've added UnregisteredPartException which is a subclass of the ParseFailedException.
It has an additional field part_name so you can get what was the name of the unregistered field.

StreamingFormDataParser init function has an extra keyword argument strict which defaults to False.
And it passes this argument to ctor of _Parser.

In pyx file. I've added a new number in ErrorGroup. _Parser has 2 new fields, strict and unknown_part.
The latter one is public, ctor sets the strict mode. And when we are switching the active target, if the new
target is an unregistered one, we set unknown_part , mark_error and return new code from the enum.

@siddhantgoel I would appreciate if you could take a look at it :)

Copy link
Owner

@siddhantgoel siddhantgoel left a comment

Choose a reason for hiding this comment

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

Overall looks like it'll work! Just left some minor naming suggestions and looks like the CI needs to be made green to be able to merge this in.

streaming_form_data/parser.py Outdated Show resolved Hide resolved
streaming_form_data/parser.py Outdated Show resolved Hide resolved
streaming_form_data/_parser.pyx Outdated Show resolved Hide resolved
streaming_form_data/_parser.pyx Outdated Show resolved Hide resolved
@hbusul
Copy link
Contributor Author

hbusul commented Sep 12, 2023

Overall looks like it'll work! Just left some minor naming suggestions and looks like the CI needs to be made green to be able to merge this in.

I'm testing locally with flask to see if I get the exception. I've done the changes you have mentioned. I guess still documentation and tests are missing. I guess I should also write documentation and tests right? It would be nice to mention in the documentation this ParseFailedException as well in addition to UnexpectedPartException

@siddhantgoel
Copy link
Owner

I guess still documentation and tests are missing. I guess I should also write documentation and tests right? It would be nice to mention in the documentation this ParseFailedException as well in addition to UnexpectedPartException

Yeah, maybe another section in docs/index.rst and at least one parser test to make sure that the exception is raised in strict mode.

@hbusul
Copy link
Contributor Author

hbusul commented Sep 13, 2023

I've added some docs and a test for testing the strict mode

@siddhantgoel
Copy link
Owner

Awesome, looks good to me! I'll merge it in and try to push a release by the coming weekend.

@siddhantgoel siddhantgoel merged commit 9fa738a into siddhantgoel:main Sep 13, 2023
12 checks passed
@hbusul
Copy link
Contributor Author

hbusul commented Sep 19, 2023

@siddhantgoel sorry to bother but did you have a chance to look at the release? Cause I would really like to use this library :D

@siddhantgoel
Copy link
Owner

No worries, thanks for the reminder as I had completely forgotten about this, so sorry about that!

But v1.13.0 should be available on PyPI now!

@hbusul
Copy link
Contributor Author

hbusul commented Sep 19, 2023

yes it is there, thanks a lot!

@hbusul hbusul deleted the 87-introduce-strict-mode branch September 19, 2023 20:39
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