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.

Sign up for GitHub

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

Jump to bottom

[CLEANUP] Refactor ::getAllDeclarationBlocks() #990

Merged
merged 2 commits into from
Feb 27, 2025
Merged

[CLEANUP] Refactor ::getAllDeclarationBlocks() #990

merged 2 commits into from
Feb 27, 2025

Conversation

oliverklee
Copy link
Contributor

@oliverklee oliverklee commented Feb 25, 2025 •
edited
Loading

Part of #994.

@oliverklee oliverklee requested a review from JakeQZ February 25, 2025 09:03
@oliverklee oliverklee self-assigned this Feb 25, 2025
@coveralls
Copy link

coveralls commented Feb 25, 2025 •
edited
Loading

Coverage Status

coverage: 54.512% (-0.3%) from 54.784%
when pulling 4a7e38c on cleanup/refactor/get-all-declaration-blocks
into 215c199 on main.

JakeQZ
JakeQZ reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Can't the method getAllDeclarationBlocks simply be moved from Document to CSSBlockList as is (continuing to use the internal allDeclarationBlocks()?

@oliverklee oliverklee force-pushed the cleanup/refactor/get-all-declaration-blocks branch 2 times, most recently from 55a0fa0 to fec6092 Compare February 25, 2025 10:10
@oliverklee
Copy link
Contributor Author

Can't the method getAllDeclarationBlocks simply be moved from Document to CSSBlockList as is (continuing to use the internal allDeclarationBlocks()?

I'm planning on removing allDeclarationBlocks as it's redundant with getAllDeclarationBlocks and also is hard to read and PHPStan-analyze due to the return parameter.

I've created #994 to list the steps of this plan.

@oliverklee oliverklee force-pushed the cleanup/refactor/get-all-declaration-blocks branch 3 times, most recently from 3cc910e to 6a13625 Compare February 25, 2025 10:52
@oliverklee
Copy link
Contributor Author

Okay, I'll mark this as draft for now and move the method up first.

@oliverklee oliverklee marked this pull request as draft February 25, 2025 10:53
@oliverklee oliverklee force-pushed the cleanup/refactor/get-all-declaration-blocks branch from 6a13625 to 3d1655e Compare February 26, 2025 08:03
@oliverklee oliverklee changed the title [CLEANUP] Refactor Document::getAllDeclarationBlocks() [CLEANUP] Refactor ::getAllDeclarationBlocks() Feb 26, 2025
@oliverklee oliverklee force-pushed the cleanup/refactor/get-all-declaration-blocks branch 2 times, most recently from 1529fd3 to 5709945 Compare February 26, 2025 18:34
@oliverklee oliverklee requested a review from JakeQZ February 26, 2025 18:35
@oliverklee oliverklee marked this pull request as ready for review February 26, 2025 18:35
JakeQZ
JakeQZ requested changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Functionally fine, but have made some suggestions for improvement.

src/CSSList/CSSBlockList.php Outdated
*/
public function getAllDeclarationBlocks(): array
{
/** @var array<int, DeclarationBlock> $result */
/** @var list<DeclarationBlock> $result */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it for now. If PHPStan complains (on a higher check level), we can re-add it.

Copy link
Contributor

@JakeQZ JakeQZ Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It may have been needed when the array was passed by reference as an argument to allDeclarationBlocks(), which, with this change, it no longer is.

src/CSSList/CSSBlockList.php Outdated
$result = [];
$this->allDeclarationBlocks($result);

foreach ($this->contents as $directSibling) {
Copy link
Contributor

@JakeQZ JakeQZ Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sibling of whom? Direct (usu. adjacent) sibling (e.g. CSS +) normally refers from one to the other. I think the original $item was fine, as we are not dealing with brotherly or sisterly relationships here - unless I've missed something.

src/CSSList/CSSBlockList.php Outdated
Comment on lines 37 to 39
foreach ($directSibling->getAllDeclarationBlocks() as $grandchild) {
$result[] = $grandchild;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

array_merge() will do the job here.

@oliverklee oliverklee force-pushed the cleanup/refactor/get-all-declaration-blocks branch from 5709945 to 4a7e38c Compare February 27, 2025 08:34
@oliverklee oliverklee requested a review from JakeQZ February 27, 2025 09:22
@JakeQZ JakeQZ merged commit e1f8bd3 into main Feb 27, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/refactor/get-all-declaration-blocks branch February 27, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeQZ JakeQZ JakeQZ approved these changes

Assignees

@oliverklee oliverklee

Labels
cleanup
Projects
None yet
Milestone
9.0.0: Drop support for PHP < 7.2 and...
Development

Successfully merging this pull request may close these issues.
3 participants
@oliverklee @coveralls @JakeQZ