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
Update count param consistently. #31227
Conversation
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.
LGTM :-)
Some checks are not passing—looks unrelated, may just need a rebase and a few more spins of the wheel to get them passing (but holding on merging until they are green).
As far as I can see, this count param is not really needed, but it will still be good to keep it updated in case its getting used in an edge case that I have not considered.
2fa8e96
to
2013a66
Compare
✍🏽 Looks like the failing check is unrelated and also is solved by this PR. |
@@ -270,7 +270,7 @@ public static function parse_file_path( $file_path ) { | |||
); | |||
|
|||
$count = 0; | |||
$file_path = str_replace( array_keys( $replacements ), array_values( $replacements ), $file_path ); | |||
$file_path = str_replace( array_keys( $replacements ), array_values( $replacements ), $file_path, $count ); |
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.
With this, we no longer need to explicitly initialize $count
to zero (line 272) but I think it's fine/actually makes things a little clearer.
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.
Nice 👍
Hi @barryhughes, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
All Submissions:
Changes proposed in this Pull Request:
As far as I can see, this count param is not really needed, but it will still be good to keep it updated in case its getting used in an edge case that I have not considered.
How to test the changes in this Pull Request:
This is more of a consistency fix. Make sure all test case pass.
Other information:
Changelog entry
FOR PR REVIEWER ONLY: