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

Add suppport for [of S]? part in nth-child's arguments #120

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

annbgn
Copy link
Contributor

@annbgn annbgn commented Jul 13, 2021

Closes #76.

cssselect/parser.py Outdated Show resolved Hide resolved
@annbgn annbgn force-pushed the add_support_for_of_s_part_in_nth_child branch from 5ac2aa8 to c416f4b Compare July 21, 2021 20:36
@annbgn
Copy link
Contributor Author

annbgn commented Jul 21, 2021

i know the tests will be failing, but since i promised to have an update today, here it is :)

@annbgn annbgn force-pushed the add_support_for_of_s_part_in_nth_child branch from c416f4b to 2722ae6 Compare July 26, 2021 22:08
@annbgn annbgn force-pushed the add_support_for_of_s_part_in_nth_child branch from 4872d29 to 76b78cd Compare July 26, 2021 22:24
@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #120 (44aeedd) into master (f1fcf2b) will decrease coverage by 0.54%.
The diff coverage is 96.18%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
- Coverage   96.12%   95.58%   -0.55%     
==========================================
  Files           2        2              
  Lines         878      929      +51     
  Branches      153      166      +13     
==========================================
+ Hits          844      888      +44     
- Misses         18       23       +5     
- Partials       16       18       +2     
Impacted Files Coverage Δ
cssselect/parser.py 96.10% <94.63%> (-0.96%) ⬇️
cssselect/xpath.py 94.67% <98.85%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@annbgn annbgn force-pushed the add_support_for_of_s_part_in_nth_child branch from 97951eb to 058b6b5 Compare August 4, 2021 07:26
@wRAR
Copy link
Member

wRAR commented Aug 6, 2021

@annbgn @elacuesta do we still need anything else done here?

@annbgn annbgn marked this pull request as ready for review August 6, 2021 21:31
@annbgn
Copy link
Contributor Author

annbgn commented Aug 6, 2021

do we still need anything else done here?

imho no
i undrafted this pr

@elacuesta elacuesta changed the title WIP: add suppport for [of S]? part in nth-child's arguments Add suppport for [of S]? part in nth-child's arguments Aug 9, 2021
@@ -165,14 +165,14 @@ def __repr__(self):
return "%s[::%s(%r)]" % (
self.__class__.__name__,
self.name,
[token.value for token in self.arguments],
[token.value for token in self.arguments[0]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line rang a bell to me. With no other changes to the class, it means that the arguments argument is now expected to be formatted differently, which breaks backward compatibility. Consider the following diff:

diff --git cssselect/parser.py cssselect/parser.py
index 4351f07..48e7604 100644
--- cssselect/parser.py
+++ cssselect/parser.py
@@ -160,6 +160,7 @@ class FunctionalPseudoElement(object):
     def __init__(self, name, arguments):
         self.name = ascii_lower(name)
         self.arguments = arguments
+        print("arguments:", arguments)
 
     def __repr__(self):
         return "%s[::%s(%r)]" % (

Then at the current master branch (9edc6c3):

In [1]: from cssselect import parse

In [2]: p = parse("a::asdf(arg1)")
arguments: [<IDENT 'arg1' at 8>]

While at the current add_support_for_of_s_part_in_nth_child branch (058b6b5):

In [1]: from cssselect import parse

In [2]: p = parse("a::asdf(arg1)")
arguments: ([<IDENT 'arg1' at 8>], None)

Could you explain the rationale of this change? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the backward compatibility breaks, because I changed signature of method parse_arguments which is used both for functions and functional pseudo elements.
Now I splitted parsing arguments into separate methods, so nothing will break
Nice catch!

@elacuesta elacuesta closed this Aug 18, 2021
@elacuesta elacuesta reopened this Aug 18, 2021
@Gallaecio Gallaecio requested a review from elacuesta October 25, 2022 10:53
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.

Support :nth-child(An+B of S)
4 participants