itemadapter

[ejulio]

Just moved @elacuesta's changes here. Since we changed a few things since then, moving the commits would be a bit messy. So, I copied his changes and gave his credit in the commit message :)

[codecov[bot]]

Codecov Report

Merging #13 into master will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #13   +/-   ##
=======================================
  Coverage   98.00%   98.00%           
=======================================
  Files           4        4           
  Lines         251      251           
=======================================
  Hits          246      246           
  Misses          5        5           

Impacted Files	Coverage Δ
itemloaders/__init__.py	97.38% <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 37a63cd...ad543c1. Read the comment docs.

[ejulio]

@kmike I was thinking to raise an specific exception in __init__ if we fail to create default_item_class. What do you think? Even though it is documented, it is a bit hidden, so I'd raise a TypeError with a message similar to the one in the API Reference (maybe remove the example)..

[Gallaecio]

@kmike I was thinking to raise an specific exception in __init__ if we fail to create default_item_class.

I’m unsure.

On the one hand, given the long-term plan is to delay such an exception until load_item() or similar are called, it may be better to leave things as is; the dataclass-specific exception is not documented, so it’s an implementation detail, and by delaying the exception in the future we won’t be breaking the documented API.

That said, since we will nonetheless break the actual API in the future, defining an exception type now should not negatively affect users in the future. If they are catching the dataclass-specific exception now, they will need to move the exception handling either way; if we already define the exception class that we raise, then they will only need to move the code, and not also rename the exception class they catch.

[ejulio]

@Gallaecio

On the one hand, given the long-term plan is to delay such an exception until load_item() or similar are called, it may be better to leave things as is; the dataclass-specific exception is not documented, so it’s an implementation detail, and by delaying the exception in the future we won’t be breaking the documented API.

I don't think we'll be able to delay the creation. Check my last comment here https://github.com/scrapy/itemloaders/issues/14

That said, since we will nonetheless break the actual API in the future, defining an exception type now should not negatively affect users in the future. If they are catching the dataclass-specific exception now, they will need to move the exception handling either way; if we already define the exception class that we raise, then they will only need to move the code, and not also rename the exception class they catch.

I don't think users should handle this exception, they should fix it. Otherwise, they won't get the result they want, so they can't proceed with the given implementation.

[kmike]

Thank you @ejulio!

[ejulio]

YAY :tada: