Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

**For Review Only** Collect Named CSS Region Flows#4879

Closed
JeffryBooher wants to merge 20 commits into
masterfrom
jeff/css-regions-collect-flows
Closed

**For Review Only** Collect Named CSS Region Flows#4879
JeffryBooher wants to merge 20 commits into
masterfrom
jeff/css-regions-collect-flows

Conversation

@JeffryBooher

Copy link
Copy Markdown
Contributor
  • Adds a function to extract all named css region flow names.

This pull still needs to be squashed but it should be good to go.
Mostly curious about the regex being used and if there is any more efficient way to collect names.

@ghost ghost assigned redmunds Aug 22, 2013
Comment thread src/language/CSSUtils.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't we have to collect flow-from also?

You only need [a-z0-9_\-] since you have i flag used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about flow-from. Not too tough to add that but I'm not sure how the CSS hints will be used.

The flow-into is what "creates" the named flow. But I guess you may have flow-into in a different css and have several instances of "flow-from" that you've used and you want to remember that name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, just replace " " with "\s*".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@redmunds huh?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ooops. Wrong thread.

@ghost ghost assigned RaymondLim Aug 22, 2013
Comment thread src/language/CSSUtils.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Drive-by comment: removing comments first simplifies the regex to use to extract flow properties, but I don't think removing whitespace has any benefit. Actually, I think it would be faster to leave in the whitespace to reduce the amount of string rebuilding done by replace (and the amount of scanning would be equal in both cases). Maybe change this to a removeComments() function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well then the regex has to change to accommodate tabs, newlines, carriage returns, etc...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we execute the regex twice, once to get the properties and again to extract the name -- i wonder if it's better to remove the whitespace or just execute the regex twice that excludes the whitespace

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use the regex exec() method to only extract 1 (flow property) match at a time, which returns an array where the name is already parsed out. You have to setup a loop to go through each hit, but there's only a single regex. There's only a single pass, so less need to remove whitespace beforehand.

@JeffryBooher

Copy link
Copy Markdown
Contributor Author

@RaymondLim @redmunds all feedback have been addressed. Thanks @RaymondLim and @peterflynn for your input on flow-from -- tests were added and code was modified to handle flow-from as well.

@RaymondLim, @redmunds Let me know when you're done with the review and I'll rebase

@redmunds

Copy link
Copy Markdown
Contributor

Much cleaner!

Comment thread src/language/CSSUtils.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these 3 functions become StringUtils APIs? #4885 could use the remove comments one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this pull request should get merged then @redmunds can refactor them into something reusable for #4885.

We should probably just expose a single export from CSSUtils (not StringUtils) for this to reducing the content for CSS parsers because it doesn't work for JavaScript or other languages. CSS doesn't have the // comment syntax whereas JS does.

Perhaps the exported name should be something like reduceStyleSheetForRegExParsing() because it removes strings as well as comments. It's generally not a good idea to remove strings but we want to avoid parsing matches in quoted strings so that's why it's here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@redmunds I can make the change if you want -- just tell me what you want to do since #4885 is your pull request I thought I'd leave it up to you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That sounds good too. But since the APIs will stay in CSSUtils and they receive a string which might only be a single line, the names could be the same as now or get a bit shorter (like reduceForRegExParsing).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Go ahead and refactor those regex/functions into CSSUtils, and then I'll update my code to use them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that while CSS does not support // style line comments, LESS does support them. I'm not sure about SASS or scss.

@JeffryBooher

Copy link
Copy Markdown
Contributor Author

Rebased into a new pull -- closing

@JeffryBooher JeffryBooher deleted the jeff/css-regions-collect-flows branch August 23, 2013 00:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants