Skip to content

[CLEANUP] Avoid Hungarian notation in RuleSet#1004

Merged
JakeQZ merged 1 commit intomainfrom
cleanup/hungarian/ruleset
Mar 3, 2025
Merged

[CLEANUP] Avoid Hungarian notation in RuleSet#1004
JakeQZ merged 1 commit intomainfrom
cleanup/hungarian/ruleset

Conversation

@oliverklee
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

oliverklee commented Feb 26, 2025

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

@coveralls
Copy link
Copy Markdown

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
Copy Markdown
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.

Comment thread src/RuleSet/RuleSet.php Outdated
@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
Copy Markdown
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.

Comment thread src/RuleSet/RuleSet.php
Comment thread 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
Copy Markdown
Collaborator

@JakeQZ JakeQZ Feb 27, 2025

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?

Comment thread src/RuleSet/RuleSet.php Outdated
* 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
Copy Markdown
Collaborator

@JakeQZ JakeQZ Feb 27, 2025

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.

Comment thread src/RuleSet/RuleSet.php Outdated
*
* @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
Copy Markdown
Collaborator

@JakeQZ JakeQZ Feb 27, 2025

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.

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

@JakeQZ JakeQZ Feb 27, 2025

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
Copy Markdown
Collaborator Author

oliverklee commented Feb 27, 2025

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
Copy Markdown
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.

Comment thread src/RuleSet/RuleSet.php Outdated
@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
Copy Markdown
Collaborator Author

oliverklee commented Feb 27, 2025

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
Comment thread src/RuleSet/RuleSet.php Outdated
$sRule = $ruleToAdd->getRule();
if (!isset($this->rules[$sRule])) {
$this->rules[$sRule] = [];
$propertyOrRuleName = $ruleToAdd->getRule();
Copy link
Copy Markdown
Collaborator

@JakeQZ JakeQZ Mar 3, 2025

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
Copy Markdown
Collaborator Author

oliverklee commented Mar 3, 2025

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
Copy Markdown
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
@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