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 the default parameter to get_output_value #26

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

Conversation

noviluni
Copy link
Member

Hey guys! Good job decoupling this from Scrapy!

I’ve been thinking about the possibility of adding a default value for the get_output_value().

So as an example, you could do:

item_loader.get_output_value('name', 'Foobar')

and you would receive the value for name if it's available or 'Foobar' if it's not available.

The implementation seems quite simple, but there are a lot of scenarios that I tried to reproduce in the tests.

Could you?

1. Let me know if you like the idea.
I decided to implement it because I frequently find something similar to:

item_loader.get_output_value('name') or ''

2. Check the tests I added and see if it's what you expect or not.
If you see any missed case you can also let me know it.

Thanks in advance!

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #26 into master will increase coverage by 1.58%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   98.02%   99.60%   +1.58%     
==========================================
  Files           4        4              
  Lines         253      256       +3     
==========================================
+ Hits          248      255       +7     
+ Misses          5        1       -4     
Impacted Files Coverage Δ
itemloaders/__init__.py 99.36% <100.00%> (+1.94%) ⬆️
itemloaders/utils.py 100.00% <0.00%> (+3.12%) ⬆️

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 e6c9d3e...1fa0607. Read the comment docs.

itemloaders/__init__.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
@noviluni
Copy link
Member Author

Updated @Gallaecio

Copy link
Collaborator

@ejulio ejulio left a comment

Choose a reason for hiding this comment

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

I just have one consideration about the API.
In a dict, get will only return the default value if the key was not set.
Here, the default value is returned even if the field was set, but processing the value returned an "empty value" ('', 0, ...).

It looks good to me, though I think we should be clear as much as possible about this behavior.

Also, not sure if we need to add something to docs about it

@Gallaecio Gallaecio self-requested a review February 24, 2021 10:26
@Gallaecio
Copy link
Member

You are right, we definitely need to update the documentation.

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.

None yet

3 participants