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

Implemented data serialization/deserialization with marshmallow #21

Merged
merged 10 commits into from
Oct 26, 2022

Conversation

gentrexha
Copy link
Contributor

In reference to the discussion in #20 I wanted to propose to start implementing marshmallow for data serialization and deserialization. This might not be the best possible example for this as there are other more advanced functionalities that I haven't made use of.

Reference: https://marshmallow.readthedocs.io/en/stable/quickstart.html

Open for any discussion or feedback this might have.

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

I think these changes are really nice and you chose a great example to start with! It might be worth seeing how many issues arise when running mypy --strict because of this (just to get an idea of how far we can take it wrt other projects). I am also curious to know how compatible json.loads/dumps works with lists of marshmallow objects and similar for things like CSV export (i.e. as_tuple). I know that python's native dataclasses come packed with as_tuple which is a plus.

src/missing_prices.py Outdated Show resolved Hide resolved
src/missing_prices.py Outdated Show resolved Hide resolved
src/missing_prices.py Outdated Show resolved Hide resolved
src/utils.py Outdated Show resolved Hide resolved
src/utils.py Outdated
Comment on lines 88 to 96
def _deserialize(self, value, *args, **kwargs):
if hasattr(value, "lower"):
value = value.lower()
return super()._deserialize(value, *args, **kwargs)

def _serialize(self, value, attr, obj, **kwargs):
if value is None:
return ""
return str(value).lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

In both of these methods I see several unused attributes... (attr, obj, args, kwargs) are these required by the Schema interface? How might one go about handling the lint/type errors that arise from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were part of the suggested way to implement custom Fields in marshmallow: https://marshmallow.readthedocs.io/en/stable/custom_fields.html#creating-a-field-class

That's a good question. Not sure how this is possible. I'll try removing them and see if it still works.

src/utils.py Outdated Show resolved Hide resolved
@gentrexha
Copy link
Contributor Author

I've added some of your feedback into the suggestions @bh2smith. Let me know what you think about them?

@bh2smith
Copy link
Contributor

Looks much better! I think we can go with it (once CI passes).

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Can you add a test plan and see that the script still does the thing?

src/missing_prices.py Outdated Show resolved Hide resolved
src/utils.py Show resolved Hide resolved
@gentrexha gentrexha merged commit 8f46bb3 into main Oct 26, 2022
@gentrexha gentrexha deleted the marshmallow-proposal branch October 26, 2022 14:00
@bh2smith
Copy link
Contributor

one thing I just noticed is that all the "models" are now in the utils file. would probably be better to put them in a models file

@gentrexha
Copy link
Contributor Author

I agree it's definitely the right way to do so. I'll create an issue about it.

@gentrexha gentrexha mentioned this pull request Oct 26, 2022
@gentrexha
Copy link
Contributor Author

Issue can be found here.

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