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.

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

Changed JS replace function from string to regex version where needed #12508

Merged
merged 2 commits into from
Mar 10, 2020
Merged

Changed JS replace function from string to regex version where needed #12508

merged 2 commits into from
Mar 10, 2020

Conversation

echalone
Copy link
Contributor

Changed single-time javascript replacements (string version of Javascript replace function), which should actually be global replacements, to global replacements (regex version of Javascript replace function).

This is due to a quirks in javascript which is often overlooked: pure string replacements will only replace the FIRST occurrence of the string. However, most of the time you want to replace all occurences. For this you need to use the regex version of the javascript replace function. So I changed the string-replacements to regex-replacements where every occurence of a string should be replaced.

…ch should actually be global replacements, to global replacements (regex replacement).

This is due to a quirks in javascript which is often overlooked: pure string replacements will only replace the FIRST occurrence of the string. However, most of the time you want to replace all occurences. For this you need to use the regex version of the javascript replace function. So I changed the string-replacements to regex-replacements where every occurence of a string should be replaced.
@echalone echalone requested a review from jtpetty as a code owner March 10, 2020 15:51
@msftclas
Copy link

msftclas commented Mar 10, 2020

CLA assistant check
All CLA requirements met.

@echalone
Copy link
Contributor Author

I think the windows build died for some reason... it seems other pull requests have the same problem. Hope you're still seeing our pull requests, although those checks weren't "successful" ;)

@jtpetty jtpetty requested a review from damccorm March 10, 2020 16:36
Copy link
Contributor

@jtpetty jtpetty left a comment

Choose a reason for hiding this comment

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

LGTM but I would like @damccorm to take a look as well

@damccorm damccorm merged commit 80093b8 into microsoft:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants