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 trusted pseudo-classes argument #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add trusted pseudo-classes argument #39

wants to merge 1 commit into from

Conversation

tbicr
Copy link
Contributor

@tbicr tbicr commented Aug 3, 2013

It useful to wide cssselect pseudo-classes support like :not(), :nth-child(), :nth-last-child, :nth-of-type(), :nth-last-of-type() and etc. For this case pseudo-classes as :hover, :active and etc. will processed but not left in style attribute, so you must use for this keep_style_tags argument.

@@ -96,11 +104,13 @@ def __init__(self, html, base_url=None,
include_star_selectors=False,
remove_classes=True,
strip_important=True,
external_styles=None):
external_styles=None,
trusted_pseudoclasses=False):
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like the word "trusted". If that's the parameter name I'd expect the value to be a list of pseudo classes. Like:

    trusted_pseudoclasses=[':foo', ':bar']

...or something.

Perhaps you mean to call it trust_pseudoclasses=False which would make it a verb.

Copy link
Owner

Choose a reason for hiding this comment

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

Note how the parameter in that same signature is exclude_pseudoclasses. "verb_noun"

Copy link
Owner

Choose a reason for hiding this comment

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

Why would it be False by default?
Looking at your added unit test, it looks like a really useful feature that isn't controversial.
The reason exclude_pseudoclasses is True by default is because you can't keep pseudo classes in style attributes so keeping them is kinda silly.

@peterbe
Copy link
Owner

peterbe commented Mar 28, 2014

Your code has diverged from master. Can you rebase up your code and push back into the pull request?

@tbicr
Copy link
Contributor Author

tbicr commented Mar 29, 2014

I rebase code, fix test_selector to is_valid_selector (thanks), trusted_pseudoclasses to trust_pseudoclasses (sorry english is not my first language), but now this pull request tests broken, because sheet = cssutils.parseString(css_body) do not support test selectors. So with current cssutils no reason merge this patch.

trust_pseudoclasses is False by default to no change previous behaviour.

@peterbe
Copy link
Owner

peterbe commented Mar 30, 2014

Great work! I'm currently a bit swamped as tomorrow is the start of the new quarter but I've starred this on my todo list and I'll try to see what I can do to make it work again.

@@ -8,7 +8,7 @@

import cssutils
from lxml import etree
from lxml.cssselect import CSSSelector
from lxml.cssselect import CSSSelector, SelectorError
Copy link
Owner

Choose a reason for hiding this comment

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

What version of lxml added SelectorError?
Once we know that setup.py needs to be modified and look something like lxml>=x.y.z

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SelectorError available in lxml=>3.0 version.

@peterbe
Copy link
Owner

peterbe commented Apr 2, 2014

Test fail quite hard.
See https://gist.github.com/peterbe/9927429

Would you mind looking at that first?

It useful to wide cssselect pseudo-classes support like `:not()`, `:nth-child()`, `:nth-last-child`, `:nth-of-type()`, `:nth-last-of-type()` and etc.  For this case pseudo-classes as `:hover`, `:active` and etc. will processed but not left in style attribute, so you must use for this `keep_style_tags` argument.
@tbicr
Copy link
Contributor Author

tbicr commented Apr 2, 2014

As I said before issue in code sheet = cssutils.parseString(css_body), because it not recognize test selector: div:not(:nth-child(1)) which normal for lxml. So issue more in cssutils then premailer, but my patch was broken after adding cssutils.

I created issue for cssutils: https://bitbucket.org/cthedot/cssutils/issue/43/two-nested-pseudo-classes-with-brackets.
I can change on test_trusted_pseudo current to more easy selector div:not(:first) which will be pass, but div:not(:nth-child(1)) still will be skipped untill it will not be fixed on cssutils.

PS. I fixed test_trusted_pseudo_bad_selector because div:not(:not(:first-child)) already bad selector and filtered with cssselect, so look like SelectorError checking is duplicate of cssselect filtering bad selectors logic.

@peterbe
Copy link
Owner

peterbe commented Aug 22, 2014

Any chance we can revive this?

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