Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

clean up regexes for accuracy and internal consistency #376

Closed
wants to merge 1 commit into from

3 participants

@Dakta

Not just a separate commit, there's some other changes and improvements to some of the regexes in here to make them better reflect possible acceptable property values.

@chromakode chromakode commented on the diff
r2/r2/lib/cssfilter.py
@@ -67,13 +67,13 @@
'color': '{x11color}|{csscolor}',
'bg-gradient': r'none|{color}|[a-z-]*-gradient\(.*\)',
- 'bg-gradients': r'{bg-gradient}(?:,\s*{bg-gradient})*',
+ 'bg-gradients': r'({bg-gradient}{w},{w})*{bg-gradient}',

Why move the optional/*'d portion to the beginning of the regex?

@Dakta
Dakta added a note

It's purely style-related change for internal consistency. It's done that way in other regexes in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chromakode chromakode commented on the diff
r2/r2/lib/cssfilter.py
((6 lines not shown))
- 'border-radius': r'(({length}|{percentage}){w}){1,2}',
+ 'border-radius': r'(({length}|{percentage}){w}/?{w})?({length}|{percentage})',

It looks like this adds the / radius-width / radius-height. Shorthand. Could you please put it in a separate commit after your cleanup?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chromakode chromakode commented on the diff
r2/r2/lib/cssfilter.py
((9 lines not shown))
'single-text-shadow': r'({color}\s+)?{length}\s+{length}(\s+{length})?|{length}\s+{length}(\s+{length})?(\s+{color})?',
- 'box-shadow-pos': r'{length}\s+{length}(\s+{length})?(\s+{length})?',
+ 'box-shadow-pos': r'{length}{w}{length}({w}{length})?',

This changes the semantics from required whitespace to optional whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chromakode chromakode commented on the diff
r2/r2/lib/cssfilter.py
@@ -88,7 +88,7 @@
'background': r'{bg-gradients}',
'background-image': r'{bg-gradients}',
'background-color': r'{color}',
- 'background-position': r'(({percentage}|{length}){0,3})?\s*(top|center|left)?\s*(left|center|right)?',
+ 'background-position': r'(({percentage}|{length}){w}(left|top|center|right|bottom)?{w}){0,2}',

This makes the ({percentage}|{length}) clause non-optional. This regex matches lengths and sides, whereas I believe the spec requires a length or a side.

http://www.w3.org/TR/CSS21/colors.html#propdef-background-position

@Dakta
Dakta added a note

Indeed it does, my mistake, forgot a ? in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@spladug
Owner

Much of this will be changing around as we upgrade the filter and fix things up. I'm going to close it. Thanks for trying to clean things up!

@spladug spladug closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2012
  1. @Dakta
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 4 deletions.
  1. +4 −4 r2/r2/lib/cssfilter.py
View
8 r2/r2/lib/cssfilter.py
@@ -67,13 +67,13 @@
'color': '{x11color}|{csscolor}',
'bg-gradient': r'none|{color}|[a-z-]*-gradient\(.*\)',
- 'bg-gradients': r'{bg-gradient}(?:,\s*{bg-gradient})*',
+ 'bg-gradients': r'({bg-gradient}{w},{w})*{bg-gradient}',

Why move the optional/*'d portion to the beginning of the regex?

@Dakta
Dakta added a note

It's purely style-related change for internal consistency. It's done that way in other regexes in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- 'border-radius': r'(({length}|{percentage}){w}){1,2}',
+ 'border-radius': r'(({length}|{percentage}){w}/?{w})?({length}|{percentage})',

It looks like this adds the / radius-width / radius-height. Shorthand. Could you please put it in a separate commit after your cleanup?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
'single-text-shadow': r'({color}\s+)?{length}\s+{length}(\s+{length})?|{length}\s+{length}(\s+{length})?(\s+{color})?',
- 'box-shadow-pos': r'{length}\s+{length}(\s+{length})?(\s+{length})?',
+ 'box-shadow-pos': r'{length}{w}{length}({w}{length})?',

This changes the semantics from required whitespace to optional whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
custom_values = {
@@ -88,7 +88,7 @@
'background': r'{bg-gradients}',
'background-image': r'{bg-gradients}',
'background-color': r'{color}',
- 'background-position': r'(({percentage}|{length}){0,3})?\s*(top|center|left)?\s*(left|center|right)?',
+ 'background-position': r'(({percentage}|{length}){w}(left|top|center|right|bottom)?{w}){0,2}',

This makes the ({percentage}|{length}) clause non-optional. This regex matches lengths and sides, whereas I believe the spec requires a length or a side.

http://www.w3.org/TR/CSS21/colors.html#propdef-background-position

@Dakta
Dakta added a note

Indeed it does, my mistake, forgot a ? in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# http://www.w3.org/TR/css3-background/#border-top-right-radius
'border-radius': r'{border-radius}',
Something went wrong with that request. Please try again.