-
Notifications
You must be signed in to change notification settings - Fork 146
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
Fix param name to match the doc in SelectorList.xpath() #220
base: master
Are you sure you want to change the base?
Fix param name to match the doc in SelectorList.xpath() #220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
===========================================
- Coverage 100.00% 98.35% -1.65%
===========================================
Files 5 5
Lines 298 304 +6
Branches 51 53 +2
===========================================
+ Hits 298 299 +1
- Misses 0 4 +4
- Partials 0 1 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with this change is that it breaks backward compatibility if the parameter is specified with a keyword (i.e. .xpath(xpath=…)
).
What about making the parameter query=None
, then doing query = query or kwargs.pop('xpath', None)
, and finally raising an exception about a parameter being missing if query
is still None
at that point?
@Gallaecio I thought for a while about the compatibility thing before pushing this pr. Considering it's a trivial change, I just ignored the backward compatibility support. What about this one with backward compatibility support and deprecation warning? laggardkernel/parsel@6fa4867 |
I have minor feedback, mostly style-related, but I think that’s the best approach, yes. |
cba7a11
to
4abb905
Compare
4abb905
to
9621c01
Compare
@Gallaecio Thanks for the advice about the code style. I applied them and force pushed the commit to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we should have a test for the fix, i.e. test that passing query= works, that xpath= logs a warning, and that None raises a ValueError.
I do wonder what was the behavior before when passing None as xpath. If that did not raise a ValueError before, we may need to adapt to whatever the behavior was.
Trivial typo spotted during reading the source code.
Replace param
xpath
inSelectorList.xpath()
with namequery
to match the name used in the doc andSelector.xpath()
.