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

Add example for using dataclasses with marshmallow-dataclass #305

Merged
merged 2 commits into from Jun 16, 2022

Conversation

mmdbalkhi
Copy link
Member

@mmdbalkhi mmdbalkhi commented May 23, 2022

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code docstring.
  • Add an entry in CHANGES.md summarizing the change and linking to the issue and your username.
  • Add *Version changed* or *Version added* note in any relevant docs and docstring.
  • Run pytest and tox, no tests failed.

@mmdbalkhi mmdbalkhi changed the title Marshmallow dataclass Add support for marshmallow-dataclass May 23, 2022
@mmdbalkhi mmdbalkhi marked this pull request as ready for review May 24, 2022 17:29
src/apiflask/scaffold.py Outdated Show resolved Hide resolved
examples/dataclass/app.py Outdated Show resolved Hide resolved
examples/dataclass/app.py Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
docs/dataclass.md Outdated Show resolved Hide resolved
examples/dataclass/app.py Outdated Show resolved Hide resolved
@greyli greyli changed the title Add support for marshmallow-dataclass Support using data classes as data schema May 28, 2022
@greyli greyli added this to the 1.1.0 milestone May 28, 2022
mmdbalkhi added a commit to mmdbalkhi/apiflask- that referenced this pull request May 28, 2022
@mmdbalkhi mmdbalkhi requested a review from greyli June 7, 2022 08:05
@greyli
Copy link
Member

greyli commented Jun 11, 2022

Thank you for working on this. However, after reading marshmallow-dataclass's docs and issue list, I think it's not a good idea to add direct support in APIFlask for it. For example:

  • It generates unnecessary default description value for fields.
  • User needs to use the nested metadata keyword to pass field info and actual metadata info, it's verbose and hard to write.
  • etc.

IMO, It's not mature enough. Users can just install this library and convert the dataclasses to schema before passing it to app.input()/app.output().

Could you refactor this PR to keep only the docs and the example application?

Besides, as I said in the review comment, the custom field should be set with dataclasses.field() instead of marshmallow_dataclass.NewType(). Here is an example from marshmallow-dataclass's README:

from dataclasses import dataclass, field
import marshmallow_dataclass
import marshmallow.validate


@dataclass
class Person:
    name: str = field(
        metadata=dict(
            load_only=True, metadata=dict(description="The person's first name")
        )
    )
    height: float = field(metadata=dict(validate=marshmallow.validate.Range(min=0)))

Sorry for changing my mind, I didn't think it through when I create that issue.

@mmdbalkhi
Copy link
Member Author

np. likely I'll work on it tomorrow

This is how I found out, is it true?

  • we just keep docs and example
  • we do not need to support dataclass for built-in on the apiflask

do I have to delete previous commits or you are with Squash merging PR to main?
and What will the docs look like? like this?

@mmdbalkhi
Copy link
Member Author

For the sake of PR cleanliness, I resolve reviews

@greyli
Copy link
Member

greyli commented Jun 11, 2022

we just keep docs and example
we do not need to support dataclass for built-in on the apiflask

Yes, we introduce how to use marshmallow-dataclass with APIFlask, the docs will like this:

  • A general introduction of how it works.
  • How to install the library (pip install marshmallow-dataclass).
  • Example to create dataclasses for input and output data.
  • Example to convert the dataclasses to marshmallow schema when passing it to app.input()/app.output (i.e. PetOut.Schema).

do I have to delete previous commits or you are with Squash merging PR to main?

You can rebase the commits or I will squash them.

@mmdbalkhi
Copy link
Member Author

mmdbalkhi commented Jun 12, 2022

@greyli I've done but I can't add marshmallow-dataclass to the requirements due pip-tools have a problem with new pip versions

@netlify
Copy link

netlify bot commented Jun 12, 2022

Deploy Preview for apiflask failed.

Name Link
🔨 Latest commit f53733d
🔍 Latest deploy log https://app.netlify.com/sites/apiflask/deploys/62a750fdfec64600084c2465

@mmdbalkhi mmdbalkhi force-pushed the marshmallow-dataclass branch 2 times, most recently from 3f8fa2c to 5a18368 Compare June 12, 2022 15:15
@mmdbalkhi
Copy link
Member Author

@greyli I do not know exactly how to solve this problem. Do you have a suggestion? Requirements files are also created by pip-compile-multi.

@greyli
Copy link
Member

greyli commented Jun 13, 2022

Just revert the changes to requirements files, I will update them later.

Copy link
Member

@greyli greyli left a comment

Choose a reason for hiding this comment

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

There are some typos and grammar errors in the docs, and the example application is not actually working. Please fix these issues.

docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Outdated
pip install marshmallow-dataclass
```

when you use dataclass, allthings same as the normal schema, but your must pass `Dataclass.Schema` to some cases:
Copy link
Member

Choose a reason for hiding this comment

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

The object passed to APIFlask should always be a schema (i.e. dataclass.Schema).

docs/schema.md Outdated


@app.get('/pets/<int:pet_id>')
@app.output(Pet)
Copy link
Member

Choose a reason for hiding this comment

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

The Pet should be Pet.Schema.

examples/README.md Outdated Show resolved Hide resolved
from apiflask import APIFlask, abort
from apiflask.validators import Length, OneOf

if dataclass is None:
Copy link
Member

Choose a reason for hiding this comment

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

This if statement makes no sense.



@app.get('/pets/<int:pet_id>')
@app.output(PetOutDataclass)
Copy link
Member

Choose a reason for hiding this comment

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

The object passed to APIFlask should always be a schema (i.e. dataclass.Schema).

docs/schema.md Outdated

in this example we not need to use `Pet.Schema()` in the output decorator, because the pets object is a dataclass.

another example but with `Pet.Schema`:
Copy link
Member

Choose a reason for hiding this comment

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

The description here does not seem to be related to the following example.

def update_pet(pet_id, data):
if pet_id > len(pets) - 1:
abort(404)
data = PetInDataclass.Schema().dump(data)
Copy link
Member

Choose a reason for hiding this comment

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

It seems unnecessary to convert the data objec to dict, why don't just do this:

pets[pet_id].name = data.name 
pets[pet_id].category = data.category

@greyli greyli changed the title Support using data classes as data schema Add example for using dataclasses with marshmallow-dataclass Jun 13, 2022
mmdbalkhi added a commit to mmdbalkhi/apiflask- that referenced this pull request Jun 13, 2022
mmdbalkhi added a commit to mmdbalkhi/apiflask- that referenced this pull request Jun 14, 2022
@mmdbalkhi mmdbalkhi requested a review from greyli June 14, 2022 16:30
Copy link
Member

@greyli greyli left a comment

Choose a reason for hiding this comment

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

Please check the two remaining review comments.

@mmdbalkhi
Copy link
Member Author

Please check the two remaining review comments.

yes, sorry. I think All is done. If there is a problem, please tell me to solve it

@greyli greyli merged commit 876e1a9 into apiflask:main Jun 16, 2022
@greyli
Copy link
Member

greyli commented Jun 16, 2022

Merged, thanks!

@mmdbalkhi mmdbalkhi deleted the marshmallow-dataclass branch June 16, 2022 14:18
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.

Add support for marshmallow-dataclass
2 participants