Skip to content

Conversation

@laggardkernel
Copy link

Trivial typo spotted during reading the source code.

Replace param xpath in SelectorList.xpath() with name query to match the name used in the doc and Selector.xpath().

class SelectorList(list):
    ...
-    def xpath(self, xpath, namespaces=None, **kwargs):
+    def xpath(self, query, namespaces=None, **kwargs):
        """
        ...
        ``query`` is the same argument as the one in :meth:`Selector.xpath`
        ...
        """
-        return self.__class__(flatten([x.xpath(xpath, namespaces=namespaces, **kwargs) for x in self]))
+        return self.__class__(flatten([x.xpath(query, namespaces=namespaces, **kwargs) for x in self]))
    ...

class Selector(object):
    ...
    def xpath(self, query, namespaces=None, **kwargs):
        ...

@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #220 (7cab4ce) into master (80509e4) will decrease coverage by 1.64%.
The diff coverage is 37.50%.

❗ Current head 7cab4ce differs from pull request most recent head 9621c01. Consider uploading reports for the commit 9621c01 to get more accurate results
Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
parsel/selector.py 96.91% <37.50%> (-3.09%) ⬇️

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 80509e4...9621c01. Read the comment docs.

Copy link
Member

@Gallaecio Gallaecio left a 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?

@laggardkernel
Copy link
Author

@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

@Gallaecio
Copy link
Member

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.

@laggardkernel laggardkernel force-pushed the bugfix/refactor-param-for-doc branch from cba7a11 to 4abb905 Compare July 15, 2021 08:13
@laggardkernel laggardkernel force-pushed the bugfix/refactor-param-for-doc branch from 4abb905 to 9621c01 Compare July 15, 2021 08:22
@laggardkernel
Copy link
Author

@Gallaecio Thanks for the advice about the code style. I applied them and force pushed the commit to this PR.

Copy link
Member

@Gallaecio Gallaecio left a 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.

@Gallaecio Gallaecio self-requested a review July 15, 2021 09:13
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.

3 participants