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

[MRG+1] default return value for extract_first #1145

Merged
merged 2 commits into from May 21, 2015
Merged

[MRG+1] default return value for extract_first #1145

merged 2 commits into from May 21, 2015

Conversation

@bosnjak
Copy link

@bosnjak bosnjak commented Apr 10, 2015

It would be useful to have a default return value for extract_first, to avoid additional data type checks.

For example, when expecting a string, this would raise an exception:
something = selector.extract_first().lower()

Allowing an optional default value can solve this elegantly:
something = selector.extract_first(default='').lower()

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Apr 10, 2015

I like it.

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Apr 13, 2015

@bosnj Simple test plz 👍

@curita
Copy link
Member

@curita curita commented Apr 13, 2015

I like it too. Should we add a default to re_first as well?

Documenting it would be good too, a simple use-case (like the one on this issue's description) in docs/topics/selectors.rst after introducing .extract_first() should be enough.

@kmike
Copy link
Member

@kmike kmike commented Apr 13, 2015

I'm fine with this feature.

  1. It needs tests.
  2. I think that we should use keyword arguments in examples and documentation .

selector.extract_first('').lower() doesn't look good to me, but selector.extract_first(default='').lower() does.

@kmike
Copy link
Member

@kmike kmike commented Apr 13, 2015

Also, it may be a right time to start adding docstrings to new methods/features.

for x in self:
return x.extract()
return x.extract() or default

This comment has been minimized.

@dangra

dangra Apr 29, 2015
Member

It doesn't look good to me. It is returning the default value only if first matched selection is an empty string.

I think the default value should be used only when there aren't matches like in:

for x in self:
    return x.extract()
else:
    return default
@bosnjak
Copy link
Author

@bosnjak bosnjak commented May 4, 2015

Thanks @dangra, updated as per your suggestion.
Also added a test case and a mention in the docs.

@curita
Copy link
Member

@curita curita commented May 5, 2015

LGTM. I'd like the same feature in re_first but we can implement it in another pull request.

@curita curita changed the title default return value for extract_first [MRG+1] default return value for extract_first May 5, 2015
kmike added a commit that referenced this pull request May 21, 2015
[MRG+1] default return value for extract_first
@kmike kmike merged commit cc2258b into scrapy:master May 21, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike
Copy link
Member

@kmike kmike commented May 21, 2015

let's sneak this in :)

@dangra
Copy link
Member

@dangra dangra commented May 21, 2015

🐞juice x 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants