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 selecting elements by namespace (#1811) #1847

Closed
wants to merge 3 commits into from
Closed

Add selecting elements by namespace (#1811) #1847

wants to merge 3 commits into from

Conversation

mo20053444
Copy link

@mo20053444 mo20053444 commented Oct 1, 2022

This commit enables users to select all elements within a particular namespace by inputting queries like "ns|*".

@mo20053444 mo20053444 changed the title Allow selecting elements by namespace (#1811) Add selecting elements by namespace (#1811) Oct 1, 2022
@@ -162,6 +162,8 @@ private void findElements() {
byId();
else if (tq.matchChomp("."))
byClass();
else if (tq.toString().endsWith("|*"))
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the right way to do this. The TokenQueue must be consumed token by token. With this implementation, queries like ns|* div (or anything after the namespace wildcard selector) will fail. You will need an appropriate matcher in TokenQueue. I think you could update the current tag matcher.

@@ -262,6 +264,12 @@ private void byTag() {
}
}

private void byNamespace() {
Copy link
Owner

Choose a reason for hiding this comment

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

I think I would just extend the byTag method to support this.

@@ -1319,6 +1319,16 @@ public void testNamespacedElements() {
assertEquals("html > body > fb|comments", els.get(0).cssSelector());
}

@Test
public void testSelectByNamespace() {
Copy link
Owner

Choose a reason for hiding this comment

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

This should be in SelectorTest, and needs to be more detailed -- e.g. to catch the example I pointed out, and validate other variations work.

Copy link
Owner

@jhy jhy left a comment

Choose a reason for hiding this comment

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

Review notes inline.

@mo20053444
Copy link
Author

@jhy Hi Jonathan, hope you had a great holiday season and thank you for your feedback! I will take a look at your comments and make changes accordingly. Thanks!

@mo20053444 mo20053444 closed this by deleting the head repository Nov 3, 2023
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.

2 participants