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

support dict as Schema #423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

OmmyZhang
Copy link

@OmmyZhang OmmyZhang commented Nov 28, 2022

Sometimes it's not necessary to define a schema class for each API.

This PR support @blp.arguments({'arg': ma.fields.Str()}) and @blp.response(200, {'arg': ma.fields.Str()})

close #180 #238

@OmmyZhang OmmyZhang changed the title support dict as Schema [WIP] support dict as Schema Nov 28, 2022
@OmmyZhang OmmyZhang changed the title [WIP] support dict as Schema support dict as Schema Nov 28, 2022
@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 99.87% // Head: 99.87% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (f75770e) compared to base (5ac7296).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          13       13           
  Lines         823      829    +6     
  Branches      180      182    +2     
=======================================
+ Hits          822      828    +6     
  Partials        1        1           
Impacted Files Coverage Δ
flask_smorest/arguments.py 100.00% <100.00%> (ø)
flask_smorest/utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lafrech
Copy link
Member

lafrech commented Dec 1, 2022

Thanks. This PR proves it is feasible with little modifications.

I'm not so happy with the test in resolve_schema_instance, but I don't have any obvious alternative.

Also it looks from your comment like this function accepts dictionaries already, while it was not explicitly intended to. Perhaps something I overlooked that worked by luck.

I shall look into this and perhaps rework this to make things explicit.

I's a pity we do the from_dict call in arguments while this is taken care of by webargs already, but we need it for the docs and getting it back from webargs would be tricky, if even possible. Not an issue.

@OmmyZhang
Copy link
Author

Also it looks from your comment like this function accepts dictionaries already, while it was not explicitly intended to.

Yes, I find this in f5b7212 when I try to figure out why ResponseMixin says :param schema schema|str|dict:.

Personally, I suggest changing this usages. It's so confusing.

@lafrech
Copy link
Member

lafrech commented Dec 2, 2022

Yes. There's something a bit fishy in this. Perhaps I should review it, especially now that alt_response is available.

I'd rather sort this out before going forward with this PR trying to find workarounds for a design issue that ought to be addressed first.

@lafrech
Copy link
Member

lafrech commented Dec 4, 2022

I looked into this tonight. Passing the schema doc as dict, which is a pretty uncommon use case, should probably be achieved using another parameter, like schema_doc or something.

Then any dict passed as schema would be expected to be a dict of fields to feed from_dict.

@lafrech
Copy link
Member

lafrech commented Aug 16, 2023

Back to this. Since passing doc as dict is neither really documented (apart from the docstring type) nor tested, I'm open to dropping the case. If anyone complains we can always figure out a way.

I've been trying to implement this on my own taking inspiration in this PR and I realized that generated schemas have a default name which yields warnings in apispec. See #180 (comment). Let's discuss this issue there.

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.

Using a dict of fields for arguments
2 participants