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

Delay item class object creation #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Gallaecio
Copy link
Member

Fixes #14

@Gallaecio Gallaecio changed the title WIP Delay item class object creation Aug 27, 2020
@Gallaecio
Copy link
Member Author

Reached a point where I think we need to make changes in itemadapter first. I think we need to be able to change:

ItemAdapter(self.item).get_field_meta(field_name)

So that ItemAdapter works the same getting an item class instead of an item object.

@ejulio
Copy link
Collaborator

ejulio commented Sep 2, 2020

I was thinking a bit about it and it seems to be a good solution, even though it might not solve all the cases.

It'll allow users to not set all fields as optional, but in case you build the item from multiple requests (or nested loaders), you'll still need to set these fields as optional.
In the nested loaders case, it can be a problem, as I'm setting something that shouldn't be optional as optional.
In the multiple requests one, it can be seen as proper modeling, I'm create an intermediary item and completing it later. So in this case, for a proper "model", the user should have 2 classes, one for the intermediary case and a final one for the full item.

@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #20 (a09e0d6) into master (811985a) will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #20      +/-   ##
===========================================
+ Coverage   99.60%   100.00%   +0.39%     
===========================================
  Files           4         4              
  Lines         254       269      +15     
===========================================
+ Hits          253       269      +16     
+ Misses          1         0       -1     
Impacted Files Coverage Δ
itemloaders/__init__.py 100.00% <100.00%> (+0.64%) ⬆️

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 811985a...a09e0d6. Read the comment docs.

Comment on lines -135 to -137
with this :class:`ItemLoader`. The nested loader shares the item
with the parent :class:`ItemLoader` so calls to :meth:`add_xpath`,
:meth:`add_value`, :meth:`replace_value`, etc. will behave as expected.
Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure if this should go or stay; “shares the item” is not technically accurate any more, but maybe we should simply reword it so that it indicates that load_item() returns the same object regardless of whether it is called on the original item loader or in a nested item loader.

@Gallaecio Gallaecio marked this pull request as ready for review November 18, 2020 22:41
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.

Fix empty __init__ limitation for dataclasses
2 participants