This repository has been archived by the owner. It is now read-only.

clean up regexes for accuracy and internal consistency #376

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@Dakta
Contributor

Dakta commented Mar 14, 2012

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.

@@ -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}',

This comment has been minimized.

@chromakode

chromakode Mar 15, 2012

Contributor

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

@chromakode

chromakode Mar 15, 2012

Contributor

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

This comment has been minimized.

@Dakta

Dakta Mar 15, 2012

Contributor

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

@Dakta

Dakta Mar 15, 2012

Contributor

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

- 'border-radius': r'(({length}|{percentage}){w}){1,2}',
+ 'border-radius': r'(({length}|{percentage}){w}/?{w})?({length}|{percentage})',

This comment has been minimized.

@chromakode

chromakode Mar 15, 2012

Contributor

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

@chromakode

chromakode Mar 15, 2012

Contributor

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

'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 comment has been minimized.

@chromakode

chromakode Mar 15, 2012

Contributor

This changes the semantics from required whitespace to optional whitespace.

@chromakode

chromakode Mar 15, 2012

Contributor

This changes the semantics from required whitespace to optional whitespace.

@@ -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 comment has been minimized.

@chromakode

chromakode Mar 15, 2012

Contributor

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

@chromakode

chromakode Mar 15, 2012

Contributor

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

This comment has been minimized.

@Dakta

Dakta Mar 15, 2012

Contributor

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

@Dakta

Dakta Mar 15, 2012

Contributor

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

@spladug

This comment has been minimized.

Show comment
Hide comment
@spladug

spladug Nov 20, 2012

Contributor

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!

Contributor

spladug commented Nov 20, 2012

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 Nov 20, 2012

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.