Skip to content

[CLEANUP] Avoid Hungarian notation in RuleSet #1004

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
Merged
merged 1 commit into from
Mar 3, 2025
Merged

[CLEANUP] Avoid Hungarian notation in RuleSet #1004

merged 1 commit into from
Mar 3, 2025

Conversation

oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Feb 26, 2025 •
edited
Loading

Part of #756

@oliverklee oliverklee requested a review from JakeQZ February 26, 2025 10:31
@oliverklee oliverklee self-assigned this Feb 26, 2025
@oliverklee
Copy link
Collaborator Author

I'm not sure about the naming at all and would appreciate a critical eye (and suggestions).

@coveralls
Copy link

coveralls commented Feb 26, 2025 •
edited
Loading

Coverage Status

coverage: 55.749%. remained the same
when pulling d568127 on cleanup/hungarian/ruleset
into 766aaa0 on main.

@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch from 9cf23ca to 51331a3 Compare February 26, 2025 18:39
JakeQZ
JakeQZ requested changes Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@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.

I can only see one improvement on naming. W3C have changed the names of the abstract concepts over time, which has left us with some class names that no longer correspond.

@oliverklee oliverklee mentioned this pull request Feb 27, 2025
Open
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch from 51331a3 to 4a7d4cd Compare February 27, 2025 08:39
@oliverklee oliverklee requested a review from JakeQZ February 27, 2025 08:40
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch from 4a7d4cd to 9dc285e Compare February 27, 2025 09:36
JakeQZ
JakeQZ requested changes Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@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.

I may have missed some changes in previous review. I've suggested a few more naming improvements.

src/RuleSet/RuleSet.php Outdated
}
/** @var array<int, Rule> $result */
$result = [];
foreach ($this->aRules as $sName => $aRules) {
foreach ($this->rules as $ruleName => $rule) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use $propertyName here instead of $ruleName?

src/RuleSet/RuleSet.php Outdated
@@ -222,34 +223,34 @@ public function getRulesAssoc($mRule = null)
* Note: this is different from pre-v.2.0 behaviour of PHP-CSS-Parser, where passing a Rule instance would
* remove all rules with the same name. To get the old behaviour, use `removeRule($rule->getRule())`.
*
* @param Rule|string|null $mRule
* @param Rule|string|null $removalPattern
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would prefer $searchPattern again here. The rules to be removed still have to be searched for.

src/RuleSet/RuleSet.php Outdated
@@ -222,34 +223,34 @@ public function getRulesAssoc($mRule = null)
* Note: this is different from pre-v.2.0 behaviour of PHP-CSS-Parser, where passing a Rule instance would
* remove all rules with the same name. To get the old behaviour, use `removeRule($rule->getRule())`.
*
* @param Rule|string|null $mRule
* @param Rule|string|null $removalPattern
* pattern to remove. If $mRule is null, all rules are removed. If the pattern ends in a dash,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This mentions the parameter by name, and would also need updating. Though could perhaps be modified to avoid being self-referential.

src/RuleSet/RuleSet.php Outdated
$sRule = $mRule->getRule();
if (!isset($this->aRules[$sRule])) {
if ($removalPattern instanceof Rule) {
$removalRule = $removalPattern->getRule();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe $propertyToRemove instead of $removalRule.

@oliverklee oliverklee mentioned this pull request Feb 27, 2025
Open
@oliverklee
Copy link
Collaborator Author

Updated and repushed.

@oliverklee oliverklee requested a review from JakeQZ February 27, 2025 10:36
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch 2 times, most recently from 7d8c5a4 to 76753c6 Compare February 27, 2025 11:18
JakeQZ
JakeQZ requested changes Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@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.

Might have been better to have this PR in more manageable chunks. But we are where we are, and we are nearly there.

I found one more variable that could be better-renamed.

@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch from 29f1010 to 5fd54e3 Compare February 27, 2025 11:44
@oliverklee oliverklee requested a review from JakeQZ February 27, 2025 11:44
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch 2 times, most recently from 3fb7d71 to 52846cb Compare February 27, 2025 12:13
@oliverklee oliverklee marked this pull request as draft February 27, 2025 12:13
@oliverklee
Copy link
Collaborator Author

Okay, I'll split this up.

@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch 5 times, most recently from 89d6e59 to 65c893a Compare February 28, 2025 19:56
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch 4 times, most recently from baa7a5c to a9f99ca Compare March 2, 2025 14:08
JakeQZ
JakeQZ reviewed Mar 3, 2025
View reviewed changes
src/RuleSet/RuleSet.php Outdated
$sRule = $ruleToAdd->getRule();
if (!isset($this->rules[$sRule])) {
$this->rules[$sRule] = [];
$propertyOrRuleName = $ruleToAdd->getRule();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having looked into how this works and what it does, I now think just $propertyName would be better - see #1051 (review).

@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch 3 times, most recently from e0e8d4a to 163db73 Compare March 3, 2025 10:21
@oliverklee oliverklee force-pushed the cleanup/hungarian/ruleset branch from 163db73 to d568127 Compare March 3, 2025 12:49
@oliverklee oliverklee marked this pull request as ready for review March 3, 2025 12:49
@oliverklee oliverklee requested a review from JakeQZ March 3, 2025 12:49
@oliverklee
Copy link
Collaborator Author

All the split-off PRs now are merged, and this PR now is quite small and ready for a re-review.

JakeQZ
JakeQZ approved these changes Mar 3, 2025
View reviewed changes
Copy link
Collaborator

@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.

Good rename to avoid a variable having two (slightly different) purposes.

@JakeQZ JakeQZ merged commit b525327 into main Mar 3, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/hungarian/ruleset branch March 3, 2025 13:01
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