parsel icon indicating copy to clipboard operation
parsel copied to clipboard

Fix param name to match the doc in SelectorList.xpath()

Open laggardkernel opened this issue 4 years ago • 4 comments
trafficstars

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):
        ...

laggardkernel avatar Jul 03 '21 10:07 laggardkernel

Codecov Report

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

:exclamation: 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%) :arrow_down:

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.

codecov[bot] avatar Jul 12 '21 08:07 codecov[bot]

@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@6fa4867ef0adc4026d9a5883b237d1b7153c2340

laggardkernel avatar Jul 15 '21 02:07 laggardkernel

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.

Gallaecio avatar Jul 15 '21 06:07 Gallaecio

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

laggardkernel avatar Jul 15 '21 08:07 laggardkernel