-
Notifications
You must be signed in to change notification settings - Fork 145
[TASK] Use delegation for DeclarationBlock
-> RuleSet
#1194
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
base: main
Are you sure you want to change the base?
Conversation
7800603
to
73956e9
Compare
c1c27a9
to
3a278e0
Compare
c935f2c
to
1f908cf
Compare
Would be good to resolve #1247 before moving on with this. |
1f908cf
to
1ce18b7
Compare
This is almost ready for review, but some tests need to be added. Before doing so, I'd appreciate a pre-review from you: @sabberworm, @oliverklee. I'm not keen on the having the various chaining methods in For This is the only realistic way forward I see for now, in an 'Agile' sense, which can deliver what is required now, without wholescale refactoring or breaking changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach!
Maybe as another spin-off PR (or prepatch), we can introduce the interface and have one class implement it. |
I think I have somehow managed to rebase without including any changes from If I could somehow reset the branch to the state it was in immediately after the commit "Reorder construction", then squashing may be possible, but I'm not sure how to do that. |
|
184f865
to
34c8708
Compare
git reset -hard {SHA-of-last-commit-before-attempted-rebase} ... undid the failed rebase/merge. I think when I tried merging instead (via GitHub), that moved the 'common ancestor' point, but when I reverted the merge, it was not moved back.
git reset -soft {SHA-of-first-branch-commit}
git commit --amend ... did the squashing.
That helped immensely; thanks. Instead of (presumably) 9 rounds of conflict resolution, including for files with no overall conflict, there was just one round, for the two files that actually had conflicts. (I'm used to Perforce, which does the merge all at once, and also has a visual editor for 3-way merging. Keeping a cool head also helps ;)) Rebase now done !) Thanks <3 |
I use PhpStorm as my IDE, and IIRC it also have a nice 3-way-merge UI. (Disclaimer: I don't use UIs for git stuff and hence can only report what I hear and see from other people.) |
... adding internal `render` method. Precursor to #1194.
76f0b5f
to
6d615c9
Compare
... instead of `getAllRuleSets`. This avoids testing if `RuleSet`s are `DeclarationBlock`s, and will be needed for #1194.
... instead of `getAllRuleSets`. This avoids testing if `RuleSet`s are `DeclarationBlock`s, and will be needed for #1194.
6d615c9
to
38c7836
Compare
I think this is now ready for a final review. |
src/RuleSet/DeclarationBlock.php
Outdated
@@ -107,7 +132,9 @@ public static function parse(ParserState $parserState, ?CSSList $list = null): ? | |||
} | |||
} | |||
$result->setComments($comments); | |||
RuleSet::parseRuleSet($parserState, $result); | |||
|
|||
RuleSet::parseRuleSet($parserState, $result->ruleSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this technically works, I'd find using the getter getRuleSet()
more clean here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseRuleSet()
takes the RuleSet
argument by reference, in order to fill it in with the the parsed Rule
s, so the class property needs to be passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand it, getRuleSet()
returns the instance, not a clone of it.
(PHP by default passes object kind-of-by-reference. I explained this on page 17 of my diploma thesis back then.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I was thinking of arrays (even though I knew it was an object - my thinking cap must have been on back-to-front). Thanks for the thesis link; I will give it a read some time...
*/ | ||
public function __construct(?int $lineNumber = null) | ||
{ | ||
$this->ruleSet = new RuleSet($lineNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need unit tests that the created RuleSet
uses has line number from the DeclarationBlock
constructor, and that it defaults to null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't appear to have an existing test that RuleSet
applies the line number provided, so perhaps another pre-PR is needed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1348 for RuleSet
. Once that is approved, I can address this along similar lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added tests to cover:
getLineNumber()
returning line number passed toDeclarationBlock
constructor (ornull
by default);getRuleSet()->getLineNumber()
returning line number passed toDeclarationBlock
constructor (ornull
by default);
But perhaps 1
could be covered by another pre-PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1350 for item 1.
@@ -175,6 +202,73 @@ public function getSelectors(): array | |||
return $this->selectors; | |||
} | |||
|
|||
public function getRuleSet(): RuleSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have unit tests for the added methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRuleSet()
has some tests added for it. The methods that chain on to the equivalent RuleSet
method have tests added via the RuleContainerTest
trait (which is also used in the test case for RuleSet
). So I think all the added methods have test coverage.
38c7836
to
d6c7420
Compare
... rather than inheritance. This will allow `DeclarationBlock` to instead extend `CSSBlockList` in order to support [CSS nesting](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_nesting). This is a slightly-breaking change, since now `CSSBlockList::getAllRuleSets()` will include the `RuleSet` property of the `DeclarationBlock` instead of the `DeclarationBlock` itself. Part of #1170.
cec8497
to
85dc980
Compare
This issues from the last review should all now be resolved. |
9c1c62e
to
e666f5e
Compare
... rather than inheritance.
This will allow
DeclarationBlock
to instead extendCSSBlockList
in order to supportCSS nesting.
This is a slightly-breaking change, since now
CSSBlockList::getAllRuleSets()
will include theRuleSet
property of theDeclarationBlock
instead of theDeclarationBlock
itself.Part of #1170.