Skip to content

Conversation

@oliverklee
Copy link
Collaborator

Part of #756

@coveralls
Copy link

coveralls commented Feb 23, 2025

Coverage Status

coverage: 53.778%. remained the same
when pulling c90e96c on cleanup/hungarian/string
into ce0a049 on main.

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 think a better rename is possible for $sStringWrapperChar.

try {
$aSelectorParts = [];
$sStringWrapperChar = false;
$stringWrapperChar = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we're renaming this variable anyway, could we not change it to $stringWrapperCharacter in this PR?

I note a couple of other very minor issues with this section of code that are beyond the scope of this PR, but should be tidied up (and can't be until this PR is merged):

  1. The non-set value for the above variable should be null rather then false;
  2. The comparison in the elseif should be strict (using ===).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have create #975 and #976 for those two issues.

@oliverklee oliverklee requested a review from JakeQZ February 24, 2025 09:48
@JakeQZ JakeQZ mentioned this pull request Feb 24, 2025
@JakeQZ JakeQZ merged commit 6f1fc2c into main Feb 24, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/hungarian/string branch February 24, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants