scrapy / itemadapter

Common interface for data container classes

BSD 3-Clause "New" or "Revised" License

61 stars 13 forks source link

Implement support for Pydantic models #53

Closed NoelTautges closed 3 years ago

NoelTautges commented 3 years ago

This pull request implements support for Pydantic models, including tests.

Coverage:

----------- coverage: platform win32, python 3.9.5-final-0 -----------
Name                      Stmts   Miss  Cover   Missing
-------------------------------------------------------
itemadapter\__init__.py       3      0   100%
itemadapter\adapter.py      158      0   100%
itemadapter\utils.py         69      0   100%
-------------------------------------------------------
TOTAL                       230      0   100%

If this is better suited for custom registration in my own projects than a pull request, I totally understand! I just saw requested it once already (#42) and thought that since it was a theme, a pull request might be useful.

codecov[bot] commented 3 years ago

Codecov Report

Merging #53 (aa226b8) into master (367a642) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head aa226b8 differs from pull request most recent head db49147. Consider uploading reports for the commit db49147 to get more accurate results

@@            Coverage Diff            @@
##            master       #53   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          196       248   +52     
=========================================
+ Hits           196       248   +52

Impacted Files	Coverage Δ
itemadapter/adapter.py	`100.00% <100.00%> (ø)`
itemadapter/utils.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 367a642...db49147. Read the comment docs.

elacuesta commented 3 years ago

Hey, thank you very much for the effort here. Code looks good to me, I just wonder whether or not we should add it to the main package, from a maintainability perspective. I'm +0.5 on merging, let's see what other maintainers think.

elacuesta commented 3 years ago

@NoelTautges Seems like field metadata could be supported via the Field function, could you maybe add support for it?

NoelTautges commented 3 years ago

@elacuesta Sure thing! Should I add every set one except for default_factory?

Gallaecio commented 3 years ago

@NoelTautges I think it should be included (if defined). If you wonder about excluding it because callables are not serializables, I don’t think field metadata needs to be serializable. It’s not included when serializing an item, if I recall correctly.

NoelTautges commented 3 years ago

@Gallaecio D'oh, you're totally right. I'll get on it. 👍

wRAR commented 3 years ago

itemadapter/utils.py:44: error: "type" has no attribute "__fields__" [attr-defined]

NoelTautges commented 3 years ago

@wRAR Thank you for your patience! A local tox run says I fixed the issue.

NoelTautges commented 3 years ago

Would someone mind taking another look at this pull request? I fixed the typing issue.

Gallaecio commented 3 years ago

Looks good to me, great work!

NoelTautges commented 3 years ago

@Gallaecio Thank you for merging, and thank you for fixing that unreachable condition! Take care :)

elacuesta commented 3 years ago

Released as v0.3.0. Thanks for the great work!

NoelTautges commented 3 years ago

@elacuesta Thank you :D