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

Correct escaping in various locations #51554

Closed
wants to merge 4 commits into from

Conversation

josephperrott
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Aug 29, 2023
@ngbot
Copy link

ngbot bot commented Aug 29, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "mergeability" is failing
    pending forbidden labels detected: action: review
    pending status "pullapprove" is pending
    pending missing required status "ci/circleci: build"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@josephperrott josephperrott marked this pull request as ready for review August 29, 2023 17:09
@jessicajaniuk jessicajaniuk added area: testing Issues related to Angular testing features, such as TestBed area: animations area: core Issues related to the framework runtime area: compiler Issues related to `ngc`, Angular's template compiler labels Aug 29, 2023
@ngbot ngbot bot added this to the Backlog milestone Aug 29, 2023
@josephperrott josephperrott removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 29, 2023
Correct various Useless regular-expression character escape issues.
Correct various Useless regular-expression character escape issues.
Correct various Useless regular-expression character escape issues.
Correct various Useless regular-expression character escape issues.
@alfaproject
Copy link
Contributor

Some of these look obviously broken or not expected and should be double-checked by the original author perhaps

If you see something like this: \s* then the author wanted it to be 0 or more whitespace characters, but now you changed it to s* which means 0 or more s characters.

So, if no tests fail and no one ever reported any issue, you can just remove the \s* altogether because it never did anything, or check with the author

@josephperrott
Copy link
Member Author

@alfaproject I agree that this does not match what the author was intending to do in many of the cases, but these \ uses were superfluous. So the resulting regexs are actually the same.

These two strings create the same RegExp when actually interpretted:

new RegExp(`^test\s`) ==> /^tests/

new RegExp(`^tests`) ==> /^tests/

@josephperrott josephperrott requested review from AndrewKushnir and removed request for crisbeto August 29, 2023 21:35
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 1baeca8.

jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…1554)

Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…51554)

Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…1554)

Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…51554)

Correct various Useless regular-expression character escape issues.

PR Close #51554
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
Correct various Useless regular-expression character escape issues.

PR Close #51554
@alfaproject
Copy link
Contributor

@josephperrott I know they do but that was not my point. Instead of fixing what the author intended or removing \s* altogether since it wasn't doing anything you left those regexes in a very weird format now....

@gultyayev
Copy link

Agree with @alfaproject. Looks like it would be better to do the \\ so that it would become the original intent of targeting a space instead of some literal "s"

@josephperrott josephperrott deleted the correct-escaping branch August 31, 2023 15:48
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 1, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…51554)

Correct various Useless regular-expression character escape issues.

PR Close angular#51554
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…gular#51554)

Correct various Useless regular-expression character escape issues.

PR Close angular#51554
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ngular#51554)

Correct various Useless regular-expression character escape issues.

PR Close angular#51554
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…lar#51554)

Correct various Useless regular-expression character escape issues.

PR Close angular#51554
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: animations area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime area: testing Issues related to Angular testing features, such as TestBed target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants