Skip to content

Conversation

@harpocrates
Copy link
Contributor

Call replace with a global regex argument so that all occurrences are replace.

Description

In #6245 I forgot that calling replace with a string as the first
argument replaces only the first occurrence of the string. In order to
replace all occurrences, we need to use a regex with the global (g)
modifier.

Sorry. 😖

Motivation and Context

Fixes escaping of curl requests that have two dollar signs (see the test for a complete example)

How Has This Been Tested?

Added a unit test

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@tim-lai
Copy link
Contributor

tim-lai commented Sep 8, 2020

@harpocrates Good catch, thanks for the update! Would you mind updating the Jest test in test/unit/core/curlify.js, which replaced the recently migrated Mocha test/mocha/core/curlify.js?

@tim-lai tim-lai self-assigned this Sep 8, 2020
In swagger-api#6245 I forgot that calling `replace` with a string as the first
argument replaces only the first occurrence of the string. In order to
replace all occurrences, we need to use a regex with the global (g)
modifier.
@harpocrates harpocrates force-pushed the alec/curlify-regex-replace branch from c32c219 to 4045dd4 Compare September 10, 2020 13:18
@harpocrates
Copy link
Contributor Author

@tim-lai should be done now, thank you!

@tim-lai tim-lai merged commit 89d57fc into swagger-api:master Sep 10, 2020
@tim-lai
Copy link
Contributor

tim-lai commented Sep 10, 2020

@harpocrates Merged! Thanks again for the contribution!

@harpocrates harpocrates deleted the alec/curlify-regex-replace branch September 10, 2020 17:22
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.

2 participants