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] Support kwargs for response.xpath() #2457

Merged
merged 4 commits into from Feb 2, 2017

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Dec 20, 2016

Allow passing named variables and namespaces dict arguments on .xpath() shortcut, now that scrapy/parsel#45 is merged

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 20, 2016

Codecov Report

Merging #2457 into master will not impact coverage.

@@           Coverage Diff           @@
##           master    #2457   +/-   ##
=======================================
  Coverage   83.49%   83.49%           
=======================================
  Files         161      161           
  Lines        8787     8787           
  Branches     1289     1289           
=======================================
  Hits         7337     7337           
  Misses       1203     1203           
  Partials      247      247
Impacted Files Coverage Δ
scrapy/http/response/text.py 97.22% <100%> (ø)

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 c305c46...1295c17. Read the comment docs.

@kmike
Copy link
Member

@kmike kmike commented Dec 21, 2016

+1 to have it; it'd be nice to have docs for this feature right in Scrapy docs.

@redapple
Copy link
Contributor Author

@redapple redapple commented Jan 11, 2017

@kmike , I updated the docs on Selectors with XPath variables.
Do you want to mention ad-hoc namespaces too?

@redapple redapple changed the title Support kwargs for response.xpath() [MRG] Support kwargs for response.xpath() Jan 31, 2017
@kmike
Copy link
Member

@kmike kmike commented Feb 2, 2017

In setup.py we declare parsel >= 0.9.5, but this feature requires parsel >= 1.1.0. Time to bump version in setup.py?

@kmike
Copy link
Member

@kmike kmike commented Feb 2, 2017

No opinion about ad-hoc namespaces, I'm fine with whatever you decide :)

@redapple
Copy link
Contributor Author

@redapple redapple commented Feb 2, 2017

Is it strictly needed? For those not using kwargs, 0.9.5<=parsel<1.1 should work fine.
I thought setup.py was about the absolute minimum required version.
No strong opinion on that though, since parsel 1.1 is better.

@kmike
Copy link
Member

@kmike kmike commented Feb 2, 2017

We have two options:

  1. document that this feature requires a more recent parsel;
  2. bump parsel version.

(2) looks like a no-brainer for me - parsel is a pure-Python package, and there are no breaking changes, while (1) requires user to think twice and check what's this 'parsel' thing and what is the installed version :)

@redapple redapple force-pushed the redapple:parsel-selector-kwargs branch from 15f1b55 to 1295c17 Feb 2, 2017
@kmike kmike merged commit ae2a529 into scrapy:master Feb 2, 2017
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 83.49%)
Details
codecov/project 83.49% (+0%) compared to c305c46
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike
Copy link
Member

@kmike kmike commented Feb 2, 2017

👍

@redapple redapple deleted the redapple:parsel-selector-kwargs branch Feb 23, 2017
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

3 participants