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

Modify unknown default to None for json, form and json_or_form #595

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Mar 12, 2021

Closes #580.

No test was broken and I didn't add any.

CHANGELOG will need to be sorted out before releasing but I did it this way to minimize merge conflicts.

@lafrech lafrech added this to the 8.0.0 milestone Mar 12, 2021
@lafrech lafrech requested a review from sirosen March 12, 2021 22:55
@sirosen
Copy link
Collaborator

sirosen commented Mar 12, 2021

I'm not sure I quite understand the mypy failure.

It looks like it's failing in cases where we've subclassed and override DEFAULT_UNKNOWN_BY_LOCATION because it deduces the type as Mapping[str, str] and then we include **Parser.DEFAULT_UNKNOWN_BY_LOCATION which now is a subtype of Mapping[str, Optional[str]]...?

Maybe the solution is just to explicitly add a type annotation in the pyramid parser and the aiohttp parser.

The change itself looks good -- exactly what we talked about and you put forth as a better default -- we just need to get the linting to agree that it's good. 😉

@lafrech lafrech force-pushed the body_args_unknown_default_none branch from c87f4ea to 1322202 Compare March 12, 2021 23:23
@lafrech
Copy link
Member Author

lafrech commented Mar 12, 2021

Yeah, I don't get it either. I suspect a mypy issue but I'm not educated enough to even file a bug report.

Maybe the solution is just to explicitly add a type annotation in the pyramid parser and the aiohttp parser.

It's late so I won't investigate any further now. I just did this and it passed.

Copy link
Collaborator

@sirosen sirosen 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! 👍

@lafrech lafrech force-pushed the body_args_unknown_default_none branch from 1322202 to 3c179c1 Compare March 15, 2021 09:08
@lafrech lafrech merged commit 8e6e68a into dev Mar 15, 2021
@lafrech lafrech deleted the body_args_unknown_default_none branch March 15, 2021 09:12
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.

[RFC] Modify unknown default to None for json, form and json_or_form
2 participants