refactor: add utility to identify non-standard ports #1616
Conversation
For nock#1404 We were doing this logic in several places and not always getting good coverage. Adding a small utility DRYs up the logic.
lib/common.js
Outdated
| port = `:${port}` | ||
| } | ||
| const { method = 'GET', path = '', port } = options | ||
| const portStr = addNonStandardPort(options.proto, port) ? `:${port}` : '' |
There was a problem hiding this comment.
What do you think about moving the comma into the return value? I think it would clean these up a bit.
There was a problem hiding this comment.
The colon?
Change the new function to always return a string? Either an empty string for the "false" case or :${port}.
Another option would be to require the host and return the host string, with the port if non standard.
lib/recorder.js
Outdated
| scope.push(':') | ||
| scope.push(options.port) | ||
| if (common.addNonStandardPort(options.proto, options.port, options.host)) { | ||
| scope.push(':', options.port) |
There was a problem hiding this comment.
If you move the comma into the return value, the if statement can be removed.
|
@paulmelnikow I expanded on your suggestion and changed it to return the whole origin since that's what all three callers were assembling anyway. |
|
This looks lovely! |
|
🎉 This PR is included in version 11.0.0-beta.24 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
For #1404 (code coverage) We were doing this logic in several places and not always getting good coverage. Adds a small utility DRY things up.
For #1404 (code coverage) We were doing this logic in several places and not always getting good coverage. Adds a small utility DRY things up.
For #1404 (code coverage) We were doing this logic in several places and not always getting good coverage. Adds a small utility DRY things up.
For #1404
We were doing this logic in several places and not always getting good coverage.
Adding a small utility DRYs it up.