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

Selector regex fix #130

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

Conversation

sbacic
Copy link

@sbacic sbacic commented Sep 30, 2016

This fix solves the following:

  1. eslint octal errors (should be in separate branch really, but I couldn't run the tests without it)
  2. faulty selector regex - previously limited to only lowercase letters, now includes all valid css classnames

Note: I didn't do negative testing, so these changes might mean that some other errant selector passes the new regex pattern and gets included when it shouldn't. But I don't think that likely.

EDIT: Hmm, apparently it now allows certain selectors when it shouldn't. Gonna fix that and reopen the PR after I make the changes.

Original regex did not cover all valid class
selectors. The new one does - it adds support
for integers, capital letters, underscores and
dashes.
@sbacic sbacic reopened this Sep 30, 2016
@sbacic sbacic closed this Sep 30, 2016
@sbacic
Copy link
Author

sbacic commented Sep 30, 2016

OK, I've added a commit that fixes that problem. Unfortunately, this breaks one of the existing tests. I'm afraid I don't have the time to hunt down this bug as well. I suspect that with the new regexp, the "join" glue is captured (which probably suggests that there might be more false positives).

@jeremypeter
Copy link

@sbacic Created sbacic#1 based on your branch that fixes the broken test and adds additional tests based on #116

@danielpost
Copy link

Is this still active? Escaped selectors still aren't being removed even when unused.

@zackkrida
Copy link

Would love to see this merged!

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.

None yet

5 participants