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

Refactor selector list and schema handling #2300

Merged
merged 4 commits into from Jan 21, 2017

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Jan 17, 2017

On the heels of the recent code cleanups, this one fixes a IMO rather big ambiguity. AST nodes holding selectors referenced the base selector class. This lead to multiple issues which were generally solved by using dynamic casts. This PR changes the implementation that Selector Schemas are always part of a Selector List (more precisely, a Selector List can have a Schema). When expanding/evaluating nodes we act accordingly. This is still not perfect, but allows us to carry a Selector List type around, instead of a base class. Since this class is final we know that it will itself evaluate to a selector list and so on.

@mgreter mgreter force-pushed the refactor/selector-schema branch 2 times, most recently from 5bd51c3 to d95df9e Compare January 17, 2017 04:20
@xzyfer xzyfer self-requested a review January 17, 2017 09:38
@xzyfer xzyfer added this to the 3.5.0.beta.2 milestone Jan 17, 2017
@mgreter mgreter force-pushed the refactor/selector-schema branch 26 times, most recently from 3cd7534 to 608edba Compare January 20, 2017 13:38
Copy link
Contributor

@xzyfer xzyfer left a comment

Choose a reason for hiding this comment

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

Sorry I had looked at this early in the week but mustn't have clicked the button.

This greatly simplifies the handling with selectors. We
always store a `Selector_List` and handle interpolation
as needed. Selectors are evaluated in expand, which might
be in a for loop. Therefore we either eval the list or
the schema to get the result (caching might be possible).
This exposed some ambiguity with the `type` method.
Renamed some methods to avoid the name clash for now.
@mgreter mgreter merged commit 10085f6 into sass:master Jan 21, 2017
@mgreter mgreter self-assigned this Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants